report: link to calculator w/ values #10754

paulirish · 2020-05-12T06:30:20Z

googlebot · 2020-05-12T06:30:28Z

☹️ Sorry, but only Googlers may change the label cla: yes.

lighthouse-core/report/html/renderer/performance-category-renderer.js

connorjclark · 2020-05-12T08:08:21Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

+        category.auditRefs.find(m => m.id === 'first-meaningful-paint'),
+      ]);
+      const metricPairs = v5andv6metrics.map(audit => {
+        const value = (audit.result.numericValue || 0).toString();


wouldnt this make a super good score for errors?

what happens if we pass along a null instead?

that

which actually isn't bad, something is obviously wrong and score goes to 0, so I agree we pass null and update score calc to show a ? or something when there's a NaN

lighthouse-core/report/html/renderer/performance-category-renderer.js

patrickhulce

very slick calc, this is going to be awesome!

lighthouse-core/report/html/renderer/performance-category-renderer.js

patrickhulce · 2020-05-12T13:42:30Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

+        return [audit.id, value];
+      });
+      const params = new URLSearchParams(metricPairs);
+      const url = new URL('https://googlechrome.github.io/lighthouse/scorecalc/');


what the... where was the PR for this how does it work already? 😆

what the... where was the PR for this how does it work already? 😆

Brendan turns on gh-pages branch protection 😆

paulirish/lh-scorecalc@f5027d1 :)

patrickhulce · 2020-05-12T13:43:42Z

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

@@ -496,7 +496,9 @@ Util.i18n = null;
 */
 Util.UIStrings = {
  /** Disclaimer shown to users below the metric values (First Contentful Paint, Time to Interactive, etc) to warn them that the numbers they see will likely change slightly the next time they run Lighthouse. */
-  varianceDisclaimer: 'Values are estimated and may vary. The [performance score is calculated](https://web.dev/performance-scoring/) directly from these metrics.',
+  varianceDisclaimer: 'Values are estimated and may vary. The [performance score is calculated](https://web.dev/performance-scoring/) directly from these metrics. ',


Suggested change

varianceDisclaimer: 'Values are estimated and may vary. The [performance score is calculated](https://web.dev/performance-scoring/) directly from these metrics. ',

varianceDisclaimer: 'Values are estimated and may vary. The [performance score is calculated](https://web.dev/performance-scoring/) directly from these metrics.',

if we need spacing between these let's do CSS rather than affect the translations? I'm nearly certain translators would not preserve this space as critical to layout :)

Could we stick these strings together? Slightly more awkward to deal with, but "See calculator" could default link to https://googlechrome.github.io/lighthouse/scorecalc/ and then performance-category-renderer could enhance that by reaching into the string span, finding the calculator link and replacing it with the parameterized link

did with css spacing

@brendankenny part 2 of this (mentioned in #10726) is to just reuse the same 'perf score is calculated' link to do all of it.

@brendankenny part 2 of this (mentioned in #10726) is to just reuse the same 'perf score is calculated' link to do all of it.

FWIW we shouldn't really concat these i18n-wise, but I guess everyone will be getting English strings until they're translated anyways :)

FWIW we shouldn't really concat these i18n-wise, but I guess everyone will be getting English strings until they're translated anyways :)

actually I still don't understand how that idea precludes doing my idea or requires the current impl in the meantime (since the base string was just changed in #10725 and won't be translated anyways), but ok :)

(since the base string was just changed in #10725 and won't be translated anyways), but ok :)

that change is a noop to the TC pipeline, in fact. :)

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

patrickhulce · 2020-05-12T13:49:35Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

+        category.auditRefs.find(m => m.id === 'first-meaningful-paint'),
+      ]);
+      const metricPairs = v5andv6metrics.map(audit => {
+        const value = (audit.result.numericValue || 0).toString();


that

which actually isn't bad, something is obviously wrong and score goes to 0, so I agree we pass null and update score calc to show a ? or something when there's a NaN

brendankenny · 2020-05-12T14:49:20Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

+      // Add link to score calculator
+      const calculatorLink = this.dom.createChildOf(estValuesEl, 'a');
+      calculatorLink.textContent = strings.calculatorLink;
+      const v5andv6metrics = /** @type {LH.ReportResult.AuditRef[]} */ ([


would be good to not override tsc here and deal with the possible undefineds. The benefit is it'll be backward and forward (when we drop these metrics) compatible :)

brendankenny · 2020-05-12T14:52:57Z

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

@@ -496,7 +496,9 @@ Util.i18n = null;
 */
 Util.UIStrings = {
  /** Disclaimer shown to users below the metric values (First Contentful Paint, Time to Interactive, etc) to warn them that the numbers they see will likely change slightly the next time they run Lighthouse. */
-  varianceDisclaimer: 'Values are estimated and may vary. The [performance score is calculated](https://web.dev/performance-scoring/) directly from these metrics.',
+  varianceDisclaimer: 'Values are estimated and may vary. The [performance score is calculated](https://web.dev/performance-scoring/) directly from these metrics. ',


Could we stick these strings together? Slightly more awkward to deal with, but "See calculator" could default link to https://googlechrome.github.io/lighthouse/scorecalc/ and then performance-category-renderer could enhance that by reaching into the string span, finding the calculator link and replacing it with the parameterized link

brendankenny · 2020-05-12T14:57:45Z

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

@@ -496,7 +496,9 @@ Util.i18n = null;
 */
 Util.UIStrings = {
  /** Disclaimer shown to users below the metric values (First Contentful Paint, Time to Interactive, etc) to warn them that the numbers they see will likely change slightly the next time they run Lighthouse. */
-  varianceDisclaimer: 'Values are estimated and may vary. The [performance score is calculated](https://web.dev/performance-scoring/) directly from these metrics.',
+  varianceDisclaimer: 'Values are estimated and may vary. The [performance score is calculated](https://web.dev/performance-scoring/) directly from these metrics. ',
+  /** Text link pointing to the Lighthouse scoring calculator. This link immediately follows a sentence stating the performance score is calculated from the perf metrics. */


I'm not sure if the second sentence adds much for the translators. Maybe something about it being an "interactive calculator that explains Lighthouse scoring"? Maybe unnecessary. But maybe add a suggestion about keeping it short?

brendankenny · 2020-05-12T14:59:22Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

@@ -152,6 +152,23 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
      const estValuesEl = this.dom.createChildOf(metricAuditsEl, 'div', 'lh-metrics__disclaimer');
      const disclaimerEl = this.dom.convertMarkdownLinkSnippets(strings.varianceDisclaimer);
      estValuesEl.appendChild(disclaimerEl);
+
+      // Add link to score calculator


wdyt about splitting out into a function getScoringCalculatorHref(auditRefs): string?

Co-authored-by: Connor Clark <[email protected]> Co-authored-by: Patrick Hulce <[email protected]>

connorjclark · 2020-05-12T19:26:53Z

i'd like the url to specify the scoring guide (6.0 vs 5.0 vs some future 8.0 ...)

and the device type (which varies the score)

both these features need to be added to the calc but that can come in the following days.

edit: #10763

paulirish · 2020-05-12T19:52:15Z

i'd like the url to specify the scoring guide (6.0 vs 5.0 vs some future 8.0 ...)

and the device type (which varies the score)

both these features need to be added to the calc but that can come in the following days.

very fair. I'll put in a second PR for review. this feature is a little more invasive than we'd like because the perf renderer otherwise doesnt know about configSettings or the version.

patrickhulce

LGTM 🎉

brendankenny

otherwise LGTM

brendankenny · 2020-05-12T20:15:51Z

lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js

+      const href = renderer._getScoringCalculatorHref(categoryClone.auditRefs);
+      expect(href).toContain('largest-contentful-paint=null');
+    });
+  });


would be good to add a check to the existing renders the metrics variance disclaimer as markdown test to check the generated DOM as well (beyond just a correct url)

connorjclark · 2020-05-13T00:14:18Z

this link needs to open in a new window

connorjclark · 2020-05-13T00:18:08Z

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

@@ -614,6 +614,10 @@
  color: var(--color-gray-700);
 }

+.lh-calclink::before {
+  content: ' ';


can you add a space another way

thank for catching. i tried a few ways and thought my padding solution was a bit extra. i remembered content but didnt test it. whoops

back to being extra.

report: link to calculator w/ values

dd3b051

paulirish requested a review from a team as a code owner May 12, 2020 06:30

paulirish requested review from patrickhulce and removed request for a team May 12, 2020 06:30

googlebot added the cla: yes label May 12, 2020

devtools-bot assigned patrickhulce May 12, 2020

devtools-bot added waiting4reviewer and removed cla: yes labels May 12, 2020

googlebot added the cla: yes label May 12, 2020

paulirish mentioned this pull request May 12, 2020

Embed scorecalc within performance-scoring doc #10726

Closed

updatejson

b3e35a6

vercel bot deployed to Preview May 12, 2020 06:51 View deployment

scorecalc now in lighthouse's ghpages

0fb9c4f

vercel bot deployed to Preview May 12, 2020 07:07 View deployment

lint

fa567cb

vercel bot deployed to Preview May 12, 2020 07:40 View deployment

paulirish added 2 commits May 12, 2020 00:55

add calclink to proto

b5a43bb

add trailing whitespace for sentence spacing

10837c1

vercel bot deployed to Preview May 12, 2020 07:58 View deployment

connorjclark requested changes May 12, 2020

View reviewed changes

patrickhulce suggested changes May 12, 2020

View reviewed changes

brendankenny reviewed May 12, 2020

View reviewed changes

paulirish added the 6.0 label May 12, 2020

Apply suggestions from code review

b2d3d26

Co-authored-by: Connor Clark <[email protected]> Co-authored-by: Patrick Hulce <[email protected]>

vercel bot deployed to Preview May 12, 2020 18:50 View deployment

paulirish added 3 commits May 12, 2020 12:06

extract to function

ca707ad

feedback

bc9b405

json

4adb1c6

vercel bot deployed to Preview May 12, 2020 19:21 View deployment

add tests

2809b96

vercel bot deployed to Preview May 12, 2020 19:46 View deployment

patrickhulce approved these changes May 12, 2020

View reviewed changes

connorjclark approved these changes May 12, 2020

View reviewed changes

brendankenny approved these changes May 12, 2020

View reviewed changes

connorjclark requested changes May 13, 2020

View reviewed changes

connorjclark mentioned this pull request May 13, 2020

report: add device type and version to scorecalc link #10763

Merged

paulirish added 3 commits May 12, 2020 18:27

open in new window

bc7ed23

space with padding not content

97a6759

check generated DOM in test. more or less

34a4c11

vercel bot deployed to Preview May 13, 2020 01:28 View deployment

paulirish added the land-when-ci-is-green label May 13, 2020

connorjclark approved these changes May 13, 2020

View reviewed changes

devtools-bot merged commit 739f21e into master May 13, 2020

devtools-bot removed the land-when-ci-is-green label May 13, 2020

brendankenny mentioned this pull request May 14, 2020

i18n(import): roundtrip to get moved strings #10785

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: link to calculator w/ values #10754

report: link to calculator w/ values #10754

paulirish commented May 12, 2020

googlebot commented May 12, 2020

connorjclark May 12, 2020

patrickhulce May 12, 2020

paulirish May 12, 2020

patrickhulce left a comment

patrickhulce May 12, 2020

brendankenny May 12, 2020

paulirish May 12, 2020

patrickhulce May 12, 2020

patrickhulce May 12, 2020

brendankenny May 12, 2020

paulirish May 12, 2020

brendankenny May 12, 2020

brendankenny May 12, 2020

paulirish May 13, 2020

patrickhulce May 12, 2020

brendankenny May 12, 2020

paulirish May 12, 2020

brendankenny May 12, 2020

brendankenny May 12, 2020

paulirish May 12, 2020

brendankenny May 12, 2020

paulirish May 12, 2020

connorjclark commented May 12, 2020 •

edited by paulirish

Loading

paulirish commented May 12, 2020

patrickhulce left a comment

brendankenny left a comment

brendankenny May 12, 2020

connorjclark commented May 13, 2020

connorjclark May 13, 2020

paulirish May 13, 2020

	varianceDisclaimer: 'Values are estimated and may vary. The [performance score is calculated](https://web.dev/performance-scoring/) directly from these metrics. ',
	varianceDisclaimer: 'Values are estimated and may vary. The [performance score is calculated](https://web.dev/performance-scoring/) directly from these metrics.',

report: link to calculator w/ values #10754

report: link to calculator w/ values #10754

Conversation

paulirish commented May 12, 2020

googlebot commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented May 12, 2020 • edited by paulirish Loading

paulirish commented May 12, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented May 12, 2020 •

edited by paulirish

Loading