report: make urls clickable #9224

connorjclark · 2019-06-17T18:46:55Z

URLs in tables and in the critical request chain details are now clickable.

NYT: https://misc-hoten.surge.sh/www.nytimes.com_2019-06-17_11-40-25.report.html

patrickhulce · 2019-06-17T21:09:11Z

lighthouse-core/report/html/renderer/details-renderer.js


    const element = this._dom.createElement('div', 'lh-text__url');
-    element.appendChild(this._renderText(displayedPath));
+    element.appendChild(this._renderAnchor(displayedPath, url));


seems like we should rename this method now ;)

maybe we can also share some logic with renderLink at this point since I was about to make _renderAnchor comments about falling back to text on invalid, etc

ah, didn't see that method. done.

lighthouse-core/report/html/report-styles.css

patrickhulce · 2019-06-17T21:09:56Z

lighthouse-core/report/html/renderer/details-renderer.js

+    const element = this._dom.createElement('a', 'lh-anchor');
+    element.textContent = text;
+    element.href = href;
+    element.target = '_blank';


don't forget rel=noopener ;)

brendankenny · 2019-06-18T19:59:40Z

lighthouse-core/report/html/renderer/details-renderer.js


    const element = this._dom.createElement('div', 'lh-text__url');
-    element.appendChild(this._renderText(displayedPath));
+    element.appendChild(this._renderLink(displayedPath, new URL(url)));


I don't think we want to move the URL construction out of _renderLink, since that's the error-prone step @patrickhulce was running into and is adding the guard against in #9067.

Can be

Suggested change

element.appendChild(this._renderLink(displayedPath, new URL(url)));

element.appendChild(this._renderLink({text: displayedPath, url}));

lighthouse-core/report/html/report-styles.css

brendankenny · 2019-06-18T20:12:33Z

lighthouse-core/report/html/templates.html

          </span>
          <span class="crc-node__tree-value">
-            <span class="crc-node__tree-file"><!-- fill me: node.request.url.file --></span>
+            <a class="lh-link crc-node__tree-file" target="_blank"><!-- fill me: node.request.url.file --></a>


obviously we do need to run lighthouse against itself, also need noopener here :)

lighthouse-core/report/html/renderer/details-renderer.js

Co-Authored-By: Brendan Kenny <[email protected]>

googlebot · 2019-06-18T21:15:53Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

paulirish

I don't think we should apply a text-decoration on hover if it's not linked.. I started looking into why that happens and then #9237 happened.

googlebot · 2019-06-20T21:56:31Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

brendankenny · 2019-06-21T01:18:16Z

lighthouse-core/report/html/report-styles.css


-.lh-text__url > .lh-text, .lh-text__url-host {
+.lh-text__url-host {
  display: inline;


@paulirish why is this a div in the first place then?

brendankenny · 2019-06-21T01:26:27Z

lighthouse-core/test/report/html/renderer/details-renderer-test.js

      assert.equal(urlEl.title, urlText);
      assert.equal(urlEl.dataset.url, urlText);
-      assert.ok(urlEl.firstChild.classList.contains('lh-text'));
+      assert.equal(urlEl.firstChild.nodeName, 'A');


can you expand the tests of this anchor (here and below) now that it's more than just an .lh-text with the url? Can probably c/p from the renders link values test (though that doesn't appear to check the .lh-text__url-host stuff?)

brendankenny · 2019-06-21T01:28:31Z

lighthouse-core/test/report/html/renderer/crc-details-renderer-test.js

-    assert.equal(chains[1].querySelector('.crc-node__tree-file').textContent, '/b.js');
-    assert.equal(chains[1].querySelector('.crc-node__tree-hostname').textContent, '(example.com)');
+    assert.equal(chains[1].querySelector('.lh-text__url a').textContent, '/b.js');
+    assert.equal(chains[1].querySelector('.lh-text__url-host').textContent, '(example.com)');


these tests should also be expanded

connorjclark · 2019-06-24T17:35:28Z

lighthouse-core/report/html/renderer/crc-details-renderer.js

    }

    // Fill in url, host, and request size information.
-    const {file, hostname} = Util.parseURL(segment.node.request.url);


tests are failing b/c of this change. DetailsRenderer.renderTextURL tosses the host part if parsed.file === '/'. Before, the chain renderer always used the results of Util.parseURL w/o post-processing. The chain renderer now looks like this when run on the root url of a site:

before:

The chain renderer now looks like this when run on the root url of a site:

this is an improvement, IMO.

googlebot · 2019-06-25T23:16:29Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

patrickhulce

LGTM!

@paulirish FYI I think your review has been addressed now that your PR has been merged

patrickhulce · 2019-06-26T01:43:42Z

lighthouse-core/report/html/renderer/crc-details-renderer.js

  }
 }

+// Alias b/c the name is really long.


😆 this is great

googlebot · 2019-06-27T03:23:12Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

connorjclark · 2019-06-27T03:23:33Z

any further change requests? @brendankenny @paulirish

connorjclark added 2 commits June 17, 2019 11:33

report: make urls clickable

aa94dee

request chain

5ebc419

connorjclark requested a review from paulirish as a code owner June 17, 2019 18:46

googlebot added the cla: yes label Jun 17, 2019

fix tests

64a7cf0

vercel bot deployed to staging June 17, 2019 20:29 View deployment

patrickhulce reviewed Jun 17, 2019

View reviewed changes

noopener

4bd8a9e

vercel bot deployed to staging June 17, 2019 23:07 View deployment

renderLink

12f4728

vercel bot deployed to staging June 17, 2019 23:13 View deployment

rename

d6eae56

vercel bot deployed to staging June 17, 2019 23:15 View deployment

fix types

fbc179f

vercel bot deployed to staging June 18, 2019 01:08 View deployment

brendankenny reviewed Jun 18, 2019

View reviewed changes

connorjclark added 2 commits June 18, 2019 13:37

remove lh-link

6c43e7e

noopener

858b63a

vercel bot deployed to staging June 18, 2019 20:38 View deployment

renderlink args

ebeda1b

vercel bot deployed to staging June 18, 2019 20:42 View deployment

brendankenny reviewed Jun 18, 2019

View reviewed changes

lighthouse-core/report/html/renderer/details-renderer.js Outdated Show resolved Hide resolved

Update lighthouse-core/report/html/renderer/details-renderer.js

a032633

Co-Authored-By: Brendan Kenny <[email protected]>

googlebot added cla: no and removed cla: yes labels Jun 18, 2019

vercel bot deployed to staging June 18, 2019 21:15 View deployment

fix test

a000cb4

vercel bot deployed to staging June 18, 2019 21:19 View deployment

fix test

425d7ec

paulirish requested changes Jun 20, 2019

View reviewed changes

report: allow CRC to use renderTextUrl (#9237)

7d93bde

vercel bot deployed to staging June 20, 2019 21:56 View deployment

googlebot added cla: no and removed cla: yes labels Jun 20, 2019

brendankenny reviewed Jun 21, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into report-url-anchors

4707e95

vercel bot deployed to staging June 24, 2019 17:10 View deployment

connorjclark commented Jun 24, 2019

View reviewed changes

detailsRenderer

c7504ab

vercel bot deployed to staging June 24, 2019 17:36 View deployment

paulirish self-assigned this Jun 25, 2019

fix test

b965f04

vercel bot deployed to staging June 25, 2019 23:13 View deployment

connorjclark added cla: yes and removed cla: no labels Jun 25, 2019

patrickhulce approved these changes Jun 26, 2019

View reviewed changes

lighthouse-core/report/html/renderer/crc-details-renderer.js

}

}

// Alias b/c the name is really long.

Copy link

Collaborator

patrickhulce Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆 this is great

test

ccefdd4

vercel bot deployed to staging June 27, 2019 03:23 View deployment

googlebot added cla: no and removed cla: yes labels Jun 27, 2019

paulirish approved these changes Jun 28, 2019

View reviewed changes

connorjclark merged commit d9e012b into master Jun 29, 2019

paulirish deleted the report-url-anchors branch June 29, 2019 00:59

connorjclark mentioned this pull request Dec 27, 2019

Don't truncate actual text (file names) #1975

Closed

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

	element.appendChild(this._renderLink(displayedPath, new URL(url)));
	element.appendChild(this._renderLink({text: displayedPath, url}));

report: make urls clickable #9224

report: make urls clickable #9224

Uh oh!

Conversation

connorjclark commented Jun 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

googlebot commented Jun 18, 2019

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jun 20, 2019

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!

googlebot commented Jun 25, 2019

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jun 27, 2019

Uh oh!

connorjclark commented Jun 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants