report(redesign): add sticky scores header #8524

connorjclark · 2019-04-22T21:02:29Z

sticky header w/ mini gauges
...appears when scrolling past score scale
...current category in view is underlined

patrickhulce · 2019-04-22T21:56:25Z

Nice!!
The only thing that jumps out to me is that the plugin gets underlined before the PWA category as you scroll and the PWA only gets underlined after you scroll past it. Seems like they should be flipped but I guess I'll find out in the code in a second :)

EDIT: Yep! it's flagged as #8524 (comment)

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

lighthouse-core/report/html/templates.html

patrickhulce · 2019-04-22T22:03:04Z

lighthouse-core/report/html/templates.html

+      // Show sticky header when the score scale begins to go underneath the topbar.
+      const showStickyHeader =
+        topbarEl.getBoundingClientRect().bottom - scoreScaleEl.getBoundingClientRect().top >= 0;
+      stickyHeaderEl.classList.toggle('stuck', showStickyHeader);


can we use lh-sticky-header--stuck? :)

patrickhulce · 2019-04-22T22:04:02Z

lighthouse-core/report/html/templates.html

+      // is the one "in view".
+      let highlightIndex = 0;
+      let highlightIndexDistance = Number.POSITIVE_INFINITY;
+      for (const [index, categoryEl] of Object.entries(document.querySelectorAll('.lh-category'))) {


I think this index-based approach is what is causing the behavior I was seeing with plugin and PWA flipped. Do we want the gauges to match their appearance order in the report instead?

just confirming you are correct in your next comment - this code will work as expected when the gauge order is as expected.

patrickhulce · 2019-04-22T22:04:24Z

lighthouse-core/report/html/templates.html

+      }
+
+      // Category order matches gauge order in sticky header.
+      // TODO(hoten): not 100% true yet, need to order gauges like: core, pwa, plugins. Remove


oh, I see :)

historically comments without a direct issue link tend to be forgotten, is that PR like the next one though?

There is a task entry in the report-redesign issue for this.

ah of course!

patrickhulce

sorry shouldn't have prematurely flushed!

lighthouse-core/report/html/templates.html

patrickhulce · 2019-04-22T22:32:48Z

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

+    // Use the middle of the viewport as an anchor - the closest category to the middle
+    // is the one "in view".
+    let highlightIndex = 0;
+    let highlightIndexDistance = Number.POSITIVE_INFINITY;


just curious, why not Infinity? :)

patrickhulce · 2019-04-22T22:45:26Z

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

+      // Normalize to middle of viewport.
+      const distanceToMiddle = categoryEl.getBoundingClientRect().top - window.innerHeight / 2;
+      // Closest negative distance to zero wins.
+      if (distanceToMiddle < 0 && highlightIndexDistance > distanceToMiddle) {


seems like we could simplify some of this but maybe I'm misunderstanding

highlightIndexDistance will only ever update when distanceToMiddle is negative

highlightIndexDistance is -distanceToMiddle, so highlightIndexDistance is always positive

highlightIndexDistance will always be greater than distanceToMiddle if distanceToMiddle is negative, so it's really just if (distanceToMiddle < 0)

this seems equivalent to "we highlight the last category that starts above the middle of the window" which at least for me is a lot easier to understand.

maybe something like the below?

const categoriesAboveTheMiddle = categories.filter(el => el.getBoundingClientRect().top - window.innerHeight / 2) const categoryToHighlight = categoriesAboveTheMiddle[categoriesAboveTheMiddle.length - 1] const guageToHighlight = gauges[categories.indexOf(categoryToHighlight)]

this works:

// Highlight mini gauge when section is in view. // In view = the last category that starts above the middle of the window. const categoryEls = Array.from(this._document.querySelectorAll('.lh-category')); const categoriesAboveTheMiddle = categoryEls.filter(el => el.getBoundingClientRect().top - window.innerHeight / 2 < 0); const highlightIndex = categoriesAboveTheMiddle.length > 0 ? categoriesAboveTheMiddle.length - 1 : 0; // Category order matches gauge order in sticky header. // TODO(hoten): not 100% true yet, need to order gauges like: core, pwa, plugins. Remove // this comment when that is done. const gaugeToHighlight = stickyHeaderEl.querySelectorAll('.lh-gauge__wrapper')[highlightIndex]; // @ts-ignore highlightEl.style.left = gaugeToHighlight.getBoundingClientRect().left + 'px';

thanks for making more declarative, i'll update

patrickhulce · 2019-04-22T22:45:47Z

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

+    // TODO(hoten): not 100% true yet, need to order gauges like: core, pwa, plugins. Remove
+    // this comment when that is done.
+    const gaugeToHighlight = stickyHeaderEl.querySelectorAll('.lh-gauge__wrapper')[highlightIndex];
+    // @ts-ignore


what are we ignoring?

Property 'style' does not exist on type 'Element' idk why

needs to be HTMLElement

patrickhulce

LGTM!

patrickhulce · 2019-04-22T23:09:28Z

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

+    // TODO(hoten): not 100% true yet, need to order gauges like: core, pwa, plugins. Remove
+    // this comment when that is done.
+    const gaugeToHighlight = stickyHeaderEl.querySelectorAll('.lh-gauge__wrapper')[highlightIndex];
+    (/** @type {HTMLElement} */ (highlightEl)).style.left =


Ah yeah I guess querySelector can return things that aren't HTMLElements, bummer! Maybe make the assertion at the top where it's declared?

just refactored to use _dom + make member vars

brendankenny · 2019-04-23T23:18:49Z

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

    this._setUpCollapseDetailsAfterPrinting();
    this._resetUIState();
    this._document.addEventListener('keydown', this.printShortCutDetect);
    this._document.addEventListener('copy', this.onCopy);
+    this._document.addEventListener('scroll', this._handleStickyHeader);
+    // window.addEventListener is undefined in jest tests.


which test file(s) fails? This sounds like jsdom wasn't set up correctly

yup that was it, fixed.

brendankenny · 2019-04-23T23:19:27Z

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

@@ -80,7 +80,7 @@ describe('ReportRenderer', () => {
      assert.ok(output.querySelector('.lh-header-sticky'), 'has a header');
      assert.ok(output.querySelector('.lh-report'), 'has report body');
      assert.equal(output.querySelectorAll('.lh-gauge__wrapper, .lh-gauge--pwa__wrapper').length,
-          sampleResults.reportCategories.length * 2, 'renders category gauges');
+          sampleResults.reportCategories.length * 3, 'renders category gauges');


maybe add a comment for where this multiplier comes from

brendankenny · 2019-04-23T23:24:52Z

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

+
+      // The sticky header is just the score gauges, but styled to be smaller. Just
+      // clone the gauges from the score header.
+      for (const gaugeWrapperEl of this._dom.findAll('.lh-gauge__wrapper', scoreHeader)) {


this feels pretty squicky to me :)

Can we make a renderScoresHeader() method and use it in both places?

brendankenny · 2019-04-23T23:41:05Z

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

@@ -380,6 +402,29 @@ class ReportUIFeatures {
    this._document.body.removeChild(a);
    setTimeout(_ => URL.revokeObjectURL(href), 500);
  }
+
+  _handleStickyHeader() {


maybe rename to make it clear this is for adjusting the sticky header on scroll events?

It handles sticky header visibility, and the highlighting. It also fires on scroll and resize.

_handleStickyHeaderVisibilityAndHighlighterOnScrollAndOnResize? :)

haha, I was thinking more like _updateStickyHeaderOnScroll or whatever :)

Arguably (it's a stretch :) resize is a special case of scrolling for this element, resizing is uncommon compared to scrolling, and the main thing is the name can communicate something about what it's up to and when

sold, to _updateStickyHeaderOnScroll

…ader

brendankenny

a few more things, but LGTM!

brendankenny · 2019-04-25T00:10:20Z

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

+  /**
+   * @param {LH.ReportResult} report
+   * @param {CategoryRenderer} categoryRenderer
+   * @param {Record<string, CategoryRenderer>} specificCategoryRenderers


categoryRenderer and specificCategoryRenderers probably need docstrings (sorry for that second name :)

brendankenny · 2019-04-25T00:11:58Z

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

-      scoreHeader.append(...defaultGauges, ...customGauges, ...pluginGauges);
-
+      scoreHeader.append(
+        ...this._renderScoreGauges(report, categoryRenderer, specificCategoryRenderers));


kind of hard to read the spread after a line break like this. Maybe save to a local var first?

brendankenny · 2019-04-25T00:12:29Z

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

+      this._dom.createChildOf(stickyHeader, 'div', 'lh-highlighter');
+
+      // The sticky header is just the score gauges, but styled to be smaller. Just
+      // clone the gauges from the score header.


not cloned anymore

brendankenny · 2019-04-25T00:12:37Z

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

+      // The sticky header is just the score gauges, but styled to be smaller. Just
+      // clone the gauges from the score header.
+      stickyHeader.append(
+        ...this._renderScoreGauges(report, categoryRenderer, specificCategoryRenderers));


same thing with the line break and spread as above

brendankenny · 2019-04-25T00:15:54Z

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

@@ -380,6 +399,29 @@ class ReportUIFeatures {
    this._document.body.removeChild(a);
    setTimeout(_ => URL.revokeObjectURL(href), 500);
  }
+
+  _updateStickyHeaderOnScroll() {


does this need to be called once on startup?

does this need to be called once on startup?

I guess not since it starts out hidden

TIL - https://stackoverflow.com/a/34095654

The scroll event does not fire on every load, only when refreshing a page that was scrolled, or when navigating to an anchor directly.

So fresh view (top of page), state should be hidden, and the default is just that. Refresh after scrolling a bit, or click on a link w/ an anchor to the report, and a scroll event fires.

brendankenny · 2019-04-25T00:16:32Z

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

+    const showStickyHeader = topbarBottom >= scoreScaleTop;
+    this.stickyHeaderEl.classList.toggle('lh-sticky-header--visible', showStickyHeader);
+
+    // Highlight mini gauge when section is in view.


early return if showStickyHeader === false?

brendankenny · 2019-04-25T00:22:29Z

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

    if (!this._dom.isDevTools()) {
      const topbarDocumentFragment = this._renderReportTopbar(report);
      reportFragment.appendChild(topbarDocumentFragment);
    }
+
+    if (scoreHeader && !this._dom.isDevTools()) {
+      const stickyHeader = this._dom.createElement('div', 'lh-sticky-header');


we need a test for this, though jsdom (AFAIK) doesn't support scrolling, so it will have to be a limited one. Since we have a nice complete "renders score gauges in this order" test, maybe just a test that .lh-sticky-header has child gauges that match the ones in .lh-scores-header?

Or something like that

a test that .lh-sticky-header has child gauges that match the ones in .lh-scores-header?

imo that doesn't seem like a useful test, since these gauges are created with the same function.

jsdom (AFAIK) doesn't support scrolling

jsdom/jsdom#1422 (comment) this makes me think it should be possible to fake it

ok it'd be way too much faking, basically all the getBoundingClientRect needs to be mocked

brendankenny · 2019-04-25T00:26:17Z

will also need a PR rename.

@paulirish do you feel comfortable with the overhead of this scroll handler? Seems comparable to the one we had before

connorjclark · 2019-04-25T01:24:05Z

Attempted a rebase - it's a lot of conflicts over many commits. Resolving them is not realistic imo.

I ran all the tests locally and they passed.

brendankenny · 2019-04-25T01:38:02Z

Attempted a rebase - it's a lot of conflicts over many commits. Resolving them is not realistic imo.

just merge? a rebase isn't necessary

I ran all the tests locally and they passed.

I got burned the last two times I overrode the Travis check, so I'd rather not do that anymore

connorjclark · 2019-04-25T01:44:41Z

Ah, failed to mention that I did merge master. Still failing.

…

On Wed, Apr 24, 2019, 6:38 PM Brendan Kenny ***@***.***> wrote: Attempted a rebase - it's a lot of conflicts over many commits. Resolving them is not realistic imo. just merge? a rebase isn't necessary I ran all the tests locally and they passed. I got burned the last two times I overrode the Travis check, so I'd rather not do that anymore — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8524 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7CAMRU6EYF7TOSNGXOYS3PSEDQBANCNFSM4HHSUZPQ> .

brendankenny · 2019-04-25T02:15:13Z

Looks like it was the NO_TRACING_STARTED flakiness. Green now

brendankenny · 2019-04-25T02:46:38Z

@paulirish had some performance feedback

paulirish · 2019-04-25T02:51:30Z

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

+    const topbarBottom = this.topbarEl.getBoundingClientRect().bottom;
+    const scoreScaleTop = this.scoreScaleEl.getBoundingClientRect().top;
+    const showStickyHeader = topbarBottom >= scoreScaleTop;
+    this.stickyHeaderEl.classList.toggle('lh-sticky-header--visible', showStickyHeader);


we'll want to change this at the bottom.

otherwise we force a layout within this function.

Interesting... The order is the same, so is the issue that code in between these two changes allows for the browser to jump in and do an extra layout? Idk how layouts work

https://developers.google.com/web/fundamentals/performance/rendering/avoid-large-complex-layouts-and-layout-thrashing

or http://wilsonpage.co.uk/preventing-layout-thrashing/

got it, makes sense.

AFAICT from the performance tab w/ this code, there are no extra layouts. One change I saw for when the sticky header toggles - "Recalculate Styles" was within the update function, but w/ your changes it ran just after on the main thread. Would like to know if I'm reading the performance timeline wrong.

Even if there technically isn't a layout cost, moving the DOM mutation to the bottom SGTM b/c it makes it obvious there is no layout thrashing.

connorjclark added 6 commits April 22, 2019 13:09

report(redesign): sticky header

9f3268e

stuck

9552d04

no align items

13467f1

highlighter

198441d

comment

e271f89

padding bottom. resize is on window.

039c2a9

connorjclark requested a review from paulirish as a code owner April 22, 2019 21:02

patrickhulce reviewed Apr 22, 2019

View reviewed changes

comment

06fe7b8

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

.lh-sticky-header--stuck

02c8970

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

report ui features

2a4c356

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

tests

7455ec4

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

patrickhulce reviewed Apr 22, 2019

View reviewed changes

declarative

da82714

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

visible

a379435

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

htmlel

fe023ee

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

patrickhulce approved these changes Apr 22, 2019

View reviewed changes

this.

e91c833

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

connorjclark mentioned this pull request Apr 22, 2019

Report redesign for v5 #8185

Closed

55 tasks

brendankenny reviewed Apr 23, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into report-ui-sticky-he…

4a3159c

…ader

fix jsdom

c99fbd7

vercel bot deployed to staging April 24, 2019 00:39 View deployment

add comment

0613ab7

vercel bot deployed to staging April 24, 2019 00:42 View deployment

_renderScoreGauges

1c369d1

vercel bot deployed to staging April 24, 2019 00:52 View deployment

_updateStickyHeaderOnScroll

23c5c2f

vercel bot deployed to staging April 24, 2019 00:54 View deployment

ws

4377ec4

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

brendankenny approved these changes Apr 25, 2019

View reviewed changes

early return, delete comment

1179d51

vercel bot deployed to staging April 25, 2019 01:02 View deployment

var

557db06

vercel bot deployed to staging April 25, 2019 01:05 View deployment

connorjclark changed the title ~~Report ui sticky header~~ report(redesign): sticky header Apr 25, 2019

brendankenny changed the title ~~report(redesign): sticky header~~ report(redesign): add sticky scores header Apr 25, 2019

paulirish reviewed Apr 25, 2019

View reviewed changes

avoid unnecessary layout

365f5e5

vercel bot deployed to staging April 25, 2019 06:57 View deployment

paulirish approved these changes Apr 25, 2019

View reviewed changes

paulirish merged commit 7ea30c7 into master Apr 25, 2019

paulirish deleted the report-ui-sticky-header branch April 25, 2019 16:42

report(redesign): add sticky scores header #8524

report(redesign): add sticky scores header #8524

Conversation

connorjclark commented Apr 22, 2019

patrickhulce commented Apr 22, 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

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

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

connorjclark Apr 24, 2019 • edited Loading

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

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 commented Apr 25, 2019

connorjclark commented Apr 25, 2019

brendankenny commented Apr 25, 2019

connorjclark commented Apr 25, 2019 via email

brendankenny commented Apr 25, 2019

brendankenny commented Apr 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

patrickhulce commented Apr 22, 2019 •

edited

Loading

connorjclark Apr 24, 2019 •

edited

Loading

connorjclark Apr 25, 2019 •

edited

Loading