report(redesign): add toggle control to show metric descriptions #8661

connorjclark · 2019-04-27T05:00:36Z

paulirish · 2019-04-29T19:49:44Z

lighthouse-core/report/html/templates.html

        </div>
        <div class=""></div>
      </div>
    </summary>
  </details>
 </template>

+<!-- Lighthouse metrics toggle -->
+<template id="tmpl-lh-metrics-toggle">
+  <div class="lh-metrics-toggle">


i'd like to use a <input type=checkbox> here (as the underlying control).

we shouldn't need JS for this guy (except for this.metricAuditGroup.classList.toggle('lh-audit-group--metrics__show-descriptions'))

this guide seems to be solid on implementation.. also including their resources at the bottom. https://webdesign.tutsplus.com/tutorials/how-to-make-custom-accessible-checkboxes-and-radio-buttons--cms-32074

done, hows that?

paulirish · 2019-04-29T19:51:35Z

lighthouse-core/report/html/templates.html

+        <defs>
+          <path id="a" d="M0 0h24v24H0V0z" />
+        </defs>
+        <clipPath id="b">


afaik this clippath isn't being used. i guess its just an artifact from sketch exports.

paulirish · 2019-04-29T19:52:52Z

lighthouse-core/report/html/templates.html

+    <div class="lh-metrics-toggle-more">
+      <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24">
+        <path class="lh-metrics-toggle-lines" d="M3 18h12v-2H3v2zM3 6v2h18V6H3zm0 7h18v-2H3v2z" />
+        <path fill="none" d="M0 0h24v24H0V0z" />


ditto with this unused path

paulirish

thx! this is def looking in the right direction.

apologies i'm finding so much in this one.

paulirish · 2019-04-29T22:16:30Z

lighthouse-core/report/html/renderer/report-ui-features.js

+    this.metricAuditGroup = this._dom.find('.lh-audit-group--metrics', this._document);
+  }
+
+  _setupMetricDescriptionToggleListeners() {


i definitely didnt expect this..

click on the newly expanded descriptions to make them go away?

this UX doesn't make sense to me

IMO this would only make sense if the toggle is scoped to just the metric title.

https://docs.google.com/document/d/15L5iaWo4ieouegpSlaqGGWpA13JnTv73xnpxuVhzkBg/edit?ts=5cc77eb3

silly that u cant link to doc comments

aiight. i'm not feeling this UX at all, so I raised a concern there: https://docs.google.com/a/google.com/document/d/15L5iaWo4ieouegpSlaqGGWpA13JnTv73xnpxuVhzkBg/edit?disco=AAAAC6njMnI

silly that u cant link to doc comments

paulirish · 2019-04-29T22:21:32Z

lighthouse-core/report/html/renderer/report-ui-features.js

+  _setupMetricDescriptionToggleListeners() {
+    const toggle = () => this.metricDescriptionToggleEl.click();
+    this.metricDescriptionToggleEl.addEventListener('input', this._toggleMetricDescription);
+    for (const el of this._dom.findAll('.lh-metric', this._document)) {


event delegation will be cleaner here.

we really only need one click listener, then just make sure the target was an .lh-metric (though it might as well be on the entire section considering how much of it lh-metrics take up...I think I agree with @paulirish about the clicking thing)

here's event delegation. and also only using the metric titles as click targets.

this.metricAuditGroup.addEventListener('click', e => { if (e.target.closest('.lh-metric__title')) this.metricDescriptionToggleEl.click() });

paulirish · 2019-04-29T22:22:26Z

lighthouse-core/report/html/renderer/report-ui-features.js

@@ -81,6 +88,8 @@ class ReportUIFeatures {
    this._document.addEventListener('copy', this.onCopy);
    this._document.addEventListener('scroll', this._updateStickyHeaderOnScroll);
    window.addEventListener('resize', this._updateStickyHeaderOnScroll);
+    this._setupMetricDescriptionToggleElements();
+    this._setupMetricDescriptionToggleListeners();


let's just merge these two

paulirish · 2019-04-29T22:27:42Z

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

@@ -102,6 +103,7 @@
  --env-item-bg: var(--color-black-100);
  --env-name-min-width: 220px;
  --env-tem-padding: 10px 0px;
+  --expandable-padding: 0 0 2px 28px;


magic number here... :/

--score-shape-margin: 7px 12px 0 4px; --score-shape-size: 12px;

afaik the 28 is 12 + 4 + 12.

I wonder if we can redefine score-shape with some base variable so these remain synchronized?

done-ish. shoot, that icon width varies based on the state ...

paulirish · 2019-04-29T22:34:37Z

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

+  fill: #7F7F7F;
+}
+.lh-metrics-toggle input:not(:checked) + label .lh-metrics-toggle-less .lh-metrics-toggle-lines,
+.lh-metrics-toggle input:checked + label .lh-metrics-toggle-more .lh-metrics-toggle-lines {


instead of this, you could override a css var up in the conditional L595 rule ...

--color-lines: var(--color-white);

paulirish · 2019-04-29T22:40:01Z

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

+.lh-metrics-toggle input:checked + label .lh-metrics-toggle-more .lh-metrics-toggle-lines {
+  fill: white;
+}
+.lh-metrics-toggle label div.lh-metrics-toggle-less {


you could afford to be more BEM-y here

.lh-metrics-toggle__icon
.lh-metrics-toggle__icon--expand

paulirish · 2019-04-29T22:40:54Z

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

+  border-radius: 20px;
+  overflow: hidden;
+}
+.lh-metrics-toggle label div {


this should be defined closer to the L605 rules. and BEMy would be .lh-metrics-toggle__icon

paulirish · 2019-04-29T22:44:40Z

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

+  opacity: 0;
+  position: absolute;
+  top: 0;
+  width: 74px;


instead of these magic numbers you could just do bottom:0; right: 0; and now its 100% 100%. ;)

also not putting left & top together in a rule? what kind of sadist are you? 😭

brendankenny

needs some tests 🎉 🎉

brendankenny · 2019-04-29T23:36:24Z

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

@@ -200,6 +200,11 @@ class CategoryRenderer {
      descriptionEl.classList.add('lh-audit-group__description');
      auditGroupHeader.appendChild(descriptionEl);
    }
+    if (group.title === 'Metrics') {


should move this over to performance-category-renderer.js. It'll have to do an extra step to re-find the header, but it would be good to keep specialized knowledge of the perf category encapsulated in that file if we can

brendankenny · 2019-04-29T23:40:13Z

lighthouse-core/report/html/renderer/report-ui-features.js

@@ -157,6 +165,24 @@ class ReportUIFeatures {
    this._copyAttempt = false;
  }

+  _setupMetricDescriptionToggleElements() {
+    this.metricDescriptionToggleEl =
+      /** @type {HTMLInputElement} */ (this._dom.find('.lh-metrics-toggle input', this._document));


will need to guard against when there's no perf category

brendankenny · 2019-04-29T23:41:04Z

lighthouse-core/report/html/renderer/report-ui-features.js

@@ -60,6 +64,9 @@ class ReportUIFeatures {
    this.expandAllDetails = this.expandAllDetails.bind(this);
    this._toggleDarkTheme = this._toggleDarkTheme.bind(this);
    this._updateStickyHeaderOnScroll = this._updateStickyHeaderOnScroll.bind(this);
+    this._toggleMetricDescription = this._toggleMetricDescription.bind(this);
+    this._setupMetricDescriptionToggleElements =


doesn't look like it need to be bound?

connorjclark · 2019-04-30T17:03:32Z

@brendankenny added tests. Also tried a tests refactor (render fn) but not sure if better

brendankenny · 2019-04-30T19:10:41Z

lighthouse-core/test/report/html/renderer/report-ui-features-test.js

+  /**
+   * @param {LH.JSON} lhr
+   */
+  function render(lhr) {


render function looks 👍

main issue is that it can be pretty slow, so we want to share rendered reports between tests as much as possible, but that's what you do here, so looks good to me

brendankenny · 2019-04-30T19:13:05Z

lighthouse-core/test/report/html/renderer/report-ui-features-test.js

-
-      assert.equal(reportUIFeatures.json, undefined);
-      reportUIFeatures.initFeatures(sampleResults);
-      assert.ok(reportUIFeatures.json);


testing json was being used as a proxy test for the reportUIFeatures being initialized, otherwise this is just testing the report renderer.

We could definitely do something much better if you want to come up with something to test :) or just restore and we'll think about it another day

How about just removing it? Doesn't seem useful now that we have all these other tests checking individual behaviors.

How about just removing it?

seems fine now, yeah

lighthouse-core/test/report/html/renderer/report-ui-features-test.js

brendankenny · 2019-04-30T19:16:33Z

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

@@ -795,6 +855,14 @@
  font-weight: bold;
 }

+/* Pushes the metric description toggle button to the right. */


not sure if it's really better down here or up with the toggle stuff

brendankenny · 2019-04-30T19:22:21Z

lighthouse-core/report/html/renderer/report-ui-features.js

+  _setupMetricDescriptionToggleListeners() {
+    const toggle = () => this.metricDescriptionToggleEl.click();
+    this.metricDescriptionToggleEl.addEventListener('input', this._toggleMetricDescription);
+    for (const el of this._dom.findAll('.lh-metric', this._document)) {


we really only need one click listener, then just make sure the target was an .lh-metric (though it might as well be on the entire section considering how much of it lh-metrics take up...I think I agree with @paulirish about the clicking thing)

paulirish · 2019-04-30T19:36:53Z

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

+}
+.lh-metrics-toggle__input:not(:checked) + label .lh-metrics-toggle__icon--less .lh-metrics-toggle__lines,
+.lh-metrics-toggle__input:checked + label .lh-metrics-toggle__icon--more .lh-metrics-toggle__lines {
+  --color-metric-toggle-lines: var(--color-white);


now you can move this prop into the L601 rule and delete this L608 rule

paulirish · 2019-04-30T19:38:23Z

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

+.lh-metrics-toggle__input:checked + label .lh-metrics-toggle__icon--more .lh-metrics-toggle__lines {
+  --color-metric-toggle-lines: var(--color-white);
+}
+.lh-metrics-toggle label .lh-metrics-toggle__icon--less {


the base .lh-metrics-toggle__icon rule could assume the --less case so this rule isnt necessary

regardless you can move width/height into the base lh-metrics-toggle__icon rule

paulirish · 2019-04-30T19:40:53Z

lighthouse-core/report/html/templates.html

+          <defs>
+            <path id="a" d="M0 0h24v24H0V0z" />
+          </defs>
+          <path class="lh-metrics-toggle__lines" d="M4 9h16v2H4zm0 4h10v2H4z" clip-path="url(#b)" />


i think you can just delete clip-path attribute and the <defs> above. it seems to be ignored.

…est.js Co-Authored-By: Hoten <cjamcl@gmail.com>

googlebot · 2019-04-30T22:11:51Z

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.

…house into report-ui-metric-desc

connorjclark · 2019-04-30T22:23:06Z

not crazy about how the tab control is styled, thoughts?

brendankenny · 2019-05-01T00:30:39Z

up to @paulirish on if he likes this :)

report(redesign): toggle metric descriptions

f46a9d2

connorjclark requested a review from paulirish as a code owner April 27, 2019 05:00

fix flex selector

c3d2435

vercel bot deployed to staging April 27, 2019 05:04 View deployment

..

e2e5525

vercel bot deployed to staging April 27, 2019 05:06 View deployment

Merge remote-tracking branch 'origin/master' into report-ui-metric-desc

e9d1b1a

vercel bot deployed to staging April 29, 2019 19:45 View deployment

paulirish requested changes Apr 29, 2019

View reviewed changes

pr

91f7da3

vercel bot deployed to staging April 29, 2019 20:44 View deployment

pr

58985f3

vercel bot deployed to staging April 29, 2019 20:47 View deployment

focus

36ed153

vercel bot deployed to staging April 29, 2019 20:50 View deployment

trim svg

ce01a5a

vercel bot deployed to staging April 29, 2019 20:51 View deployment

paulirish requested changes Apr 29, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into report-ui-metric-desc

6cfe786

vercel bot deployed to staging April 29, 2019 22:55 View deployment

combine and blur

60696ec

vercel bot deployed to staging April 29, 2019 23:02 View deployment

brendankenny requested changes Apr 29, 2019

View reviewed changes

bem

a52013e

vercel bot deployed to staging April 30, 2019 04:53 View deployment

var

99e8447

vercel bot deployed to staging April 30, 2019 04:57 View deployment

perf cat renderer

50a0a1a

vercel bot temporarily deployed to staging April 30, 2019 05:06 Inactive

Merge remote-tracking branch 'origin/master' into report-ui-metric-desc

96dd356

vercel bot deployed to staging April 30, 2019 05:07 View deployment

vars

41bb62b

vercel bot deployed to staging April 30, 2019 05:21 View deployment

tests

7989080

vercel bot deployed to staging April 30, 2019 16:46 View deployment

negative test

e8f2382

vercel bot deployed to staging April 30, 2019 16:52 View deployment

render()

9b52229

vercel bot deployed to staging April 30, 2019 17:02 View deployment

brendankenny reviewed Apr 30, 2019

View reviewed changes

paulirish reviewed Apr 30, 2019

View reviewed changes

event delegation

a9186e9

vercel bot deployed to staging April 30, 2019 22:08 View deployment

Update lighthouse-core/test/report/html/renderer/report-ui-features-t…

72078d5

…est.js Co-Authored-By: Hoten <cjamcl@gmail.com>

vercel bot deployed to staging April 30, 2019 22:11 View deployment

connorjclark added 2 commits April 30, 2019 15:20

cssssssssssssssssssssssssssssssssssslurp

a8f0ae4

Merge branch 'report-ui-metric-desc' of github.com:GoogleChrome/light…

ddcbebf

…house into report-ui-metric-desc

vercel bot deployed to staging April 30, 2019 22:20 View deployment

svggggggggggggggggggggggggggglurp

42b439c

vercel bot deployed to staging April 30, 2019 22:22 View deployment

brendankenny approved these changes May 1, 2019

View reviewed changes

paulirish approved these changes May 1, 2019

View reviewed changes

paulirish changed the title ~~report(redesign): toggle metric descriptions~~ report(redesign): add toggle control to show metric descriptions May 1, 2019

paulirish merged commit e613ba4 into master May 1, 2019

paulirish deleted the report-ui-metric-desc branch May 1, 2019 02:04

report(redesign): add toggle control to show metric descriptions #8661

report(redesign): add toggle control to show metric descriptions #8661

Conversation

connorjclark commented Apr 27, 2019

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

paulirish 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

paulirish Apr 29, 2019 • edited Loading

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

brendankenny 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

connorjclark commented Apr 30, 2019 • edited Loading

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

googlebot commented Apr 30, 2019

connorjclark commented Apr 30, 2019 • edited Loading

brendankenny commented May 1, 2019

paulirish Apr 29, 2019 •

edited

Loading

connorjclark commented Apr 30, 2019 •

edited

Loading

connorjclark commented Apr 30, 2019 •

edited

Loading