report: map metrics to audits #11732

paulirish · 2020-12-01T03:08:28Z

Adds a little filter control so the audit list in Performance can be filtered by what's relevant to each metric.

➡️ See an interactive demo here: https://lighthouse-git-mapping-googlechrome.vercel.app/english/

The UX is ~inspired by the AMP page exp tool.

Addy's doc: go/cfspg
ref #11279

patrickhulce · 2020-12-01T04:23:36Z

Sweeet! So glad we're starting down this path 🎉

My primary concern with adding the filter without reworking the opportunity display is that it seems like it suggests the impact number is for that metric. Maybe it's mostly just a wording issue? "Filter be relevant metric:" would be a step in the right direction. Another quick alternative on my mind is removing the time estimate in the opportunity section when filtered.

Other piece that stood out is the empty opportunities for most on TBT and CLS and how we can increase the prominence of diagnostics in those cases.

paulirish · 2021-04-01T20:58:00Z

I've given this a few updates. It's ready for a look. Description was updated with all the goodstuff.

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

connorjclark · 2021-04-02T19:01:34Z

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

+   * @param {LH.ReportResult.AuditRef[]} filterableMetrics
+   * @param {HTMLDivElement} element
+   */
+  renderMetricAuditFilter(filterableMetrics, element) {


Should this be in report ui features (like 3p filtering is)?

It's specific to the perf category (and the rendering of the opportunities/diagnostics), so I think it's better kept local to those methods.

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

connorjclark · 2021-04-02T19:05:53Z

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

+      if (metric.acronym === 'All') {
+        radioEl.checked = true;
+      }
+      // Dynamically write some CSS, for the CSS-only filtering


all this dynamic CSS seems much harder to follow than the equivalent JS would be. What reasons did you rule out JS?

IMO the CSS's declarative state communicates all the changes pretty clearly.
I'll give the JS equivalent a try in a second PR, for comparison.

connorjclark · 2021-04-02T19:08:08Z

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

+      .lh-filterbar__radio:checked:not(#metric-All) ~ .lh-audit-group .lh-audit,
+      .lh-filterbar__radio:checked:not(#metric-All) ~ .lh-audit-group .lh-audit-group__description,
+      .lh-filterbar__radio:checked:not(#metric-All) ~ .lh-audit-group .lh-audit-group__itemcount {
+        display: none;


Are there alternatives to hiding the filtered audits? I think keeping things in a constant place is beneficial. wdyt of reduce opacity?

we discussed this in the team meeting. there are arguments for both sides: hiding vs highlighting.

IMO highlighting is best if you're trying to do a diff. but I think the point of this control is to help focus action on the most relevant tasks. Given, that I think it's best to remove the irrelevant items from view completely.

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

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

types/lhr.d.ts

lighthouse-core/config/default-config.js

egsweeny · 2021-04-06T18:14:49Z

Overall this is looking amazing. Just a quick fly-by suggestion re: verbiage for the copy 'Filter to relevant audits:'. It wasn't intuitive to me, but the current state might hold up if we don't like my alternatives (below).

Some alternatives:

Filter audits by metric
Filter audits by applicable metric
Show audits relevant to: [alt: Filter audits relevant to:]
Show audits relevant to a specific metric:
Show audits to improve:

I'm currently leaning towards option 3, but am bike shedding and am flexible.

connorjclark · 2021-04-06T18:48:15Z

idea after talking to paul: would be nice to show metric adornments in the expanded audit view.

paulirish

idea after talking to paul: would be nice to show metric adornments in the expanded audit view.

i like this. I'm gonna do in a second PR as it adds more line changes then you'd think.

paulirish · 2021-05-12T23:34:20Z

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

+   * @param {LH.ReportResult.AuditRef[]} filterableMetrics
+   * @param {HTMLDivElement} element
+   */
+  renderMetricAuditFilter(filterableMetrics, element) {


It's specific to the perf category (and the rendering of the opportunities/diagnostics), so I think it's better kept local to those methods.

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

paulirish · 2021-05-12T23:40:26Z

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

+      if (metric.acronym === 'All') {
+        radioEl.checked = true;
+      }
+      // Dynamically write some CSS, for the CSS-only filtering


IMO the CSS's declarative state communicates all the changes pretty clearly.
I'll give the JS equivalent a try in a second PR, for comparison.

paulirish · 2021-05-12T23:41:44Z

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

+      .lh-filterbar__radio:checked:not(#metric-All) ~ .lh-audit-group .lh-audit,
+      .lh-filterbar__radio:checked:not(#metric-All) ~ .lh-audit-group .lh-audit-group__description,
+      .lh-filterbar__radio:checked:not(#metric-All) ~ .lh-audit-group .lh-audit-group__itemcount {
+        display: none;


we discussed this in the team meeting. there are arguments for both sides: hiding vs highlighting.

IMO highlighting is best if you're trying to do a diff. but I think the point of this control is to help focus action on the most relevant tasks. Given, that I think it's best to remove the irrelevant items from view completely.

types/lhr.d.ts

connorjclark · 2021-05-14T20:10:06Z

types/config.d.ts

@@ -146,6 +146,8 @@ declare global {
        id: string;
        weight: number;
        group?: string;
+        relevantAudits?: string[];


minor nit: swap order of these two new fields to match prototype field ids (and in below file)

connorjclark · 2021-05-17T20:15:55Z

lighthouse-core/lib/i18n/i18n.js

@@ -41,6 +41,10 @@ const MESSAGE_I18N_ID_REGEX = / | [^\s]+$/;
    Intl.NumberFormat = IntlPolyfill.NumberFormat;
    Intl.DateTimeFormat = IntlPolyfill.DateTimeFormat;
  }
+  // Deal with buggy regex caching. https://github.com/andyearnshaw/Intl.js/issues/308
+  if (IntlPolyfill.__disableRegExpRestore) {


admittedly i should have googled before i build node 12 with small-icu. but yes. annoying.

google-cla bot added the cla: yes label Dec 1, 2020

vercel bot deployed to Preview December 3, 2020 03:02 View deployment

paulirish added the 7.1 label Dec 14, 2020

paulirish added the P1 label Feb 11, 2021

paulirish self-assigned this Feb 11, 2021

paulirish removed the 7.1 label Feb 11, 2021

vercel bot deployed to Preview April 1, 2021 20:28 View deployment

paulirish removed their assignment Apr 1, 2021

paulirish marked this pull request as ready for review April 1, 2021 20:31

paulirish requested a review from a team as a code owner April 1, 2021 20:31

paulirish requested review from connorjclark and removed request for a team April 1, 2021 20:31

devtools-bot assigned connorjclark Apr 1, 2021

devtools-bot added the waiting4reviewer label Apr 1, 2021

connorjclark requested changes Apr 2, 2021

View reviewed changes

connorjclark added waiting4committer and removed waiting4reviewer labels Apr 2, 2021

brendankenny mentioned this pull request Apr 6, 2021

report: move budgets after diagnostics #12334

Merged

vercel bot deployed to Preview April 29, 2021 16:40 View deployment

vercel bot deployed to Preview May 12, 2021 23:20 View deployment

paulirish commented May 13, 2021

View reviewed changes

vercel bot deployed to Preview May 13, 2021 00:25 View deployment

google-cla bot added cla: no and removed cla: yes labels May 13, 2021

vercel bot deployed to Preview May 13, 2021 17:53 View deployment

vercel bot deployed to Preview May 13, 2021 18:07 View deployment

GoogleChrome deleted a comment from google-cla bot May 13, 2021

squashed commit of 'mapping-atomic' branch.

faee95c

paulirish force-pushed the mapping branch from 6e89e9d to faee95c Compare May 13, 2021 19:59

vercel bot deployed to Preview May 13, 2021 19:59 View deployment

google-cla bot added cla: yes and removed cla: no labels May 13, 2021

GoogleChrome deleted a comment from google-cla bot May 13, 2021

paulirish added waiting4reviewer and removed waiting4committer labels May 13, 2021

lint

3bb719a

vercel bot deployed to Preview May 13, 2021 20:51 View deployment

connorjclark approved these changes May 14, 2021

View reviewed changes

order swap

ed3d2c9

vercel bot deployed to Preview May 14, 2021 20:49 View deployment

paulirish added the land-when-ci-is-green label May 14, 2021

Merge remote-tracking branch 'origin/master' into mapping

86db7ec

vercel bot deployed to Preview May 17, 2021 17:55 View deployment

disable buggy regex caching in intl polyfill

cd7a638

vercel bot deployed to Preview May 17, 2021 20:08 View deployment

connorjclark reviewed May 17, 2021

View reviewed changes

semi

b3d72b0

vercel bot deployed to Preview May 17, 2021 20:18 View deployment

devtools-bot merged commit 15dad39 into master May 17, 2021

devtools-bot deleted the mapping branch May 17, 2021 20:33

devtools-bot removed the land-when-ci-is-green label May 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: map metrics to audits #11732

report: map metrics to audits #11732

report: map metrics to audits #11732

report: map metrics to audits #11732

Conversation

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