Generate DOM for markdown links in help text by ebidel · Pull Request #1979 · GoogleChrome/lighthouse

ebidel · 2017-04-07T21:00:17Z

R: all

Just support in help text / descriptions.

Will add a test once #1963 lands.

brendankenny

cooool. nice work

brendankenny · 2017-04-10T21:37:31Z

lighthouse-core/report/v2/report-renderer.js

  }

+  /**
+   * @param {string} text


@return {!HTMLSpanElement}. A function description here would be good too :)

Is the type of element important We're not specifying subclass type for the other methods.

Is the type of element important We're not specifying subclass type for the other methods.

not really, though it doesn't hurt. I'm more thinking for documentation purposes it's nice to have the information that your linkified text is coming back in a <span>.

brendankenny · 2017-04-10T21:38:03Z

lighthouse-core/report/v2/report-renderer.js

+  _convertMarkdownLinksToElement(text) {
+    const element = this._createElement('span');
+
+    // Split on markdown links (e.g. [some link](https://...)).


any reason to capture fullMarkdownLink if it's going to be unused?

brendankenny · 2017-04-10T21:46:55Z

lighthouse-core/report/v2/report-renderer.js

+
+    while (parts.length) {
+      // Remove the same number of elements as there are capture groups.
+      // eslint-disable-next-line no-unused-vars


if you want to keep the larger capture group, you can also rename fullMarkdownLink to _ to indicate that it's unused and drop the // eslint-disable-next-line no-unused-vars

ooh, can also do const [preambleText, , linkText, linkHref], though don't know how our eslint would react

i just removed the extra group. no longer needed.

brendankenny · 2017-04-10T21:47:55Z

lighthouse-core/test/report/v2/report-renderer-test.js

+  const renderer = new ReportRenderer(document);
+
+  describe('createElement', () => {
+    it('creates a simple element using default values', () => {


what's new and what's not is very confusing when you move things around :P

i like to keep you on your toes.

brendankenny · 2017-04-10T21:56:26Z

lighthouse-core/test/report/v2/report-renderer-test.js

+  });
+
+  describe('_convertMarkdownLinksToElement', () => {
+    it('converts links', () => {


A few more corner cases would be nice. With/without preamble text and text afterwards, etc

brendankenny · 2017-04-10T21:57:07Z

lighthouse-core/report/v2/report-renderer.js

+      if (linkText && linkHref) {
+        const a = this._createElement('a');
+        a.rel = 'noopener';
+        a.target = '_blank';


do we want to verify the url? Maybe someday it would be good if we start using this in more places, but today it would start checking our help text URLs, which would be nice

what do you mean by verify?

what do you mean by verify?

sorry, I meant like check that it's a valid url. a.href = new URL(linkHref).href or whatever

brendankenny · 2017-04-10T22:52:25Z

lighthouse-core/report/v2/report-renderer.js

+    // Split on markdown links (e.g. [some link](https://...)).
+    const parts = text.split(/(\[(.*?)\]\((https?:\/\/.*?)\))/g);
+
+    while (parts.length) {


I always forget that splice both creates a new array and modifies the original, so maybe change the comment to something like // Pop off the same number of elements as there are capture groups. to make it clear?

you could also avoid the splice by doing something like

let parts = text.split(/(\[(.*?)\]\((https?:\/\/.*?)\))/g); while (parts.length) { const [preambleText, fullMarkdownLink, linkText, linkHref, ...rest] = parts; // ... parts = rest; }

more explicit, though the extra assignment is kind of annoying

updated the comment.

ebidel · 2017-04-11T00:38:42Z

PTAL

brendankenny · 2017-04-11T21:48:02Z

lighthouse-core/report/v2/report-renderer.js

+        try {
+          a.href = (new URL(linkHref)).href;
+          element.appendChild(a);
+        } catch (e) {


should we fail silently here? Seems like a big enough mistake that it should complain loudly and end up in _renderException

I think it's better to fail silently here. If a link is busted for some reason, it shouldn't block the whole report from being generated.

but why is the link busted? Right now, at least, they're all coming from helpText. An error in one of those should be fixed immediately

done.

FWIW, b/c of the regex, the only time we'd get an invalid URL is if someone wrote https://. or used something crazy in help text like [boom](🍣🍺).

ebidel · 2017-04-11T23:56:41Z

This ready. Let's get it landed so other PRs can proceed.

patrickhulce · 2017-04-12T19:18:47Z

let's merge this so we can rebase #1987 and #1992 💥

brendankenny

Yeah, LGTM

ebidel added the v2report label Apr 7, 2017

ebidel added 2 commits April 7, 2017 17:17

Generate DOM for markdown links in help text

719ff97

Add test

2cb25c4

ebidel force-pushed the markdownlinks branch from c977a79 to 2cb25c4 Compare April 8, 2017 00:38

brendankenny suggested changes Apr 10, 2017

View reviewed changes

brendankenny reviewed Apr 10, 2017

View reviewed changes

ebidel added 2 commits April 10, 2017 17:17

Feedback

f3ad0be

Check for valid url

23715f8

Node doesnt have URL

692cdd0

ebidel force-pushed the markdownlinks branch from bbd5671 to 692cdd0 Compare April 11, 2017 01:46

ebidel mentioned this pull request Apr 11, 2017

Report v2: port over cards formatter #1992

Merged

brendankenny reviewed Apr 11, 2017

View reviewed changes

ebidel added 2 commits April 11, 2017 16:51

Allow to throw if invalid url

ec93a12

Verify invalid url throws

56b96e5

brendankenny reviewed Apr 12, 2017

View reviewed changes

brendankenny approved these changes Apr 12, 2017

View reviewed changes

brendankenny merged commit 87bd052 into master Apr 12, 2017

brendankenny deleted the markdownlinks branch April 12, 2017 19:31

Conversation

ebidel commented Apr 7, 2017

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebidel commented Apr 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebidel commented Apr 11, 2017

Uh oh!

patrickhulce commented Apr 12, 2017

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brendankenny Apr 10, 2017 •

edited

Loading