devtools: move topbar out of scrollable container #9077

connorjclark · 2019-05-29T18:41:09Z

Split off from #9023. The scrollbar was over the topbar:

With these changes (+ #9023):

Restructures the report like this:

- lh-root
--- topbar
--- lh-container
------ sticky
------ scores header
------ lh-report

So that lh-container can be given overflow-y: scroll.

~~Open issue: the bottom of the report is cut off - so you can't see the LH version or the link to file an issue. Any ideas on that?~~

brendankenny · 2019-05-29T20:05:36Z

Split off from #9023

what do you think about splitting all the way so we can land the progress in #9023 and then consider this rather than merging this into #9023?

connorjclark · 2019-05-29T20:28:03Z

Yeah, that's my intention. Just based it off #9023 so the diff is clear :)

patrickhulce

@hoten just a thought, does this mean we can start to unwind the scroll ancestor business from earlier and just do all our scroll logic on this one container?

If I'm understanding correctly, and that's a big if :), our report could always be height: 100% for example with our other container being the real scroll container, then feasibly we don't have to worry about how we are being embedded? Might make the seamless integrations like PSI more tricky to remember to support though

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

patrickhulce · 2019-05-30T14:58:17Z

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

@@ -238,11 +223,25 @@ class ReportRenderer {
      const stickyHeader = this._dom.createElement('div', 'lh-sticky-header');
      stickyHeader.append(
        ...this._renderScoreGauges(report, categoryRenderer, specificCategoryRenderers));
-      reportFragment.appendChild(stickyHeader);
+      container.appendChild(stickyHeader);


can we rename this var rootContainer or something while we're in here?

WDYT of this change @hoten ?

Oh, but I guess we have a separate lh-root, huh. Other name ideas but something that indicates its our root (potentially scroll) container for everything but top bar?

...reportContainer? I don't want to rename the CSS classes here tho.

reportContainer WFM!

I'm not suggesting to rename any CSS classes, just the container var

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

patrickhulce · 2019-05-30T15:00:28Z

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

    reportFragment.appendChild(container);
+    container.appendChild(headerContainer);


these two lines are the only other real significant changes? reportFragment -> rootContainer?

yes. most other stuff moved b/c of variable dependency. one thing moved because it made sense to do as setup for the function (the cat renderer stuff)

patrickhulce · 2019-05-30T15:00:49Z

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

@@ -269,7 +269,7 @@
  --header-padding: 16px 0 16px 0;
  --plugin-icon-size: 75%;
  --pwa-icon-margin: 0 7px 0 -3px;
-  --score-container-width: 92px;
+  --score-container-width: 97px;


what's the effect here/why necessary?

the text for the gauge titles was wrapping for Perf/A11y (in DevTools only) b/c the scroll bar steals some width.

patrickhulce · 2019-05-30T15:03:09Z

the bottom of the report is cut off - so you can't see the LH version or the link to file an issue. Any ideas on that?

Not sure why it's happening but can we add some padding to the bottom of the scroll container?

connorjclark · 2019-05-30T16:46:24Z

the bottom of the report is cut off - so you can't see the LH version or the link to file an issue. Any ideas on that?

Not sure why it's happening but can we add some padding to the bottom of the scroll container?

fixed by c10ea6e

connorjclark · 2019-05-30T17:21:15Z

@hoten just a thought, does this mean we can start to unwind the scroll ancestor business from earlier and just do all our scroll logic on this one container?

If I'm understanding correctly, and that's a big if :), our report could always be height: 100% for example with our other container being the real scroll container, then feasibly we don't have to worry about how we are being embedded? Might make the seamless integrations like PSI more tricky to remember to support though

I suppose... @exterkamp could talk more about PSI integration.

I've just noticed that the scroll bar is also over the topbar in the normal LH renderer (see now deployment). Just harder to notice since the scroll bar is only visible during use.

brendankenny · 2019-05-30T18:35:32Z

I've just noticed that the scroll bar is also over the topbar in the normal LH renderer (see now deployment).

wait, is that bad? Now I'm questioning what the problem was and what the fix is doing :) It seems fine in the normally rendered report to me?

connorjclark · 2019-05-30T19:24:37Z

Shipping with a scroll container by default might make using the report renderer odd... but would need to test it out across PSI/the internal demo app first to see first-hand. Maybe we could merge as-is and consider changing it later?

exterkamp · 2019-05-30T19:32:56Z

@hoten just a thought, does this mean we can start to unwind the scroll ancestor business from earlier and just do all our scroll logic on this one container?
If I'm understanding correctly, and that's a big if :), our report could always be height: 100% for example with our other container being the real scroll container, then feasibly we don't have to worry about how we are being embedded? Might make the seamless integrations like PSI more tricky to remember to support though

I suppose... @exterkamp could talk more about PSI integration.

Afaict this shouldn't be an issue for PSI. In PSI we just take the individual perf category from a PerformanceCategoryRenderer. Never mess with the header or topbar. So this shouldn't be a problem as this PR stands or if the report is made to height 100% b/c we never use a report so entirely there. 🤷‍♂️ I didn't test it thought before saying this, so 👀

patrickhulce · 2019-05-30T19:47:06Z

Maybe we could merge as-is and consider changing it later?

SGTM, twas just a thought afterall :)

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

Co-Authored-By: Patrick Hulce <[email protected]>

brendankenny · 2019-05-30T19:54:59Z

I'm not sure how to interpret that 👍 on my comment :)

If this is correct in the main report, what made the scroll bar different in devtools?

connorjclark · 2019-05-30T19:59:53Z

aahhh my emoji meant to convey that I agree that it looks fine outside DT.

The bar being over is ok to me, in DevTools it was under and looked really weird. It's hard to see in the photos in the first post, but the scrollbar in the previous version went under the topbar, the fix does not because the topbar was moved out of the scrolling element.

patrickhulce

LGTM

…se into rd-devtools-topbar

brendankenny

this solution feels a lot like one that's going to be rolled back in the next release for the actual fix (an issue in the containing panel CSS?), but in the interest of shipping, let's get it in.

connorjclark and others added 23 commits May 28, 2019 11:54

report(redesign): update styles for devtools

41aa187

sc

000f841

report ui features in devtools. add sticky header and topbar

c0bf21d

fix scroll

1e040cc

remove alt devtools header

a6e722e

fix highlighter in dt

7129ae1

comments

ca0afd1

margin for container

4a6a796

better comment

fe61b86

fix type

720dbb4

fix header check test

0153c0e

comment

009c7d2

report: increase UI density for devtools (#9070)

0ee54c8

maybe remove lh container top page margin

cc763f8

remove most of top of page shiiiit

7ace084

nearly there

6650b66

display none sticky header

8063f18

disable anim

63c9638

Merge remote-tracking branch 'origin/master' into rd-devtools

739b20a

restructure render fn

4b86fb4

toggle dark class on lh-vars el

bdbf379

comment

181a5ed

Merge remote-tracking branch 'origin/master' into rd-devtools

2031b5e

connorjclark requested a review from paulirish as a code owner May 29, 2019 18:41

googlebot added the cla: yes label May 29, 2019

comment

b2016ea

connorjclark added the 5.1 label May 29, 2019

connorjclark force-pushed the rd-devtools-topbar branch from 59f4958 to 6f892c0 Compare May 29, 2019 21:04

vercel bot deployed to staging May 30, 2019 07:52 View deployment

fix container footer cutoff

c10ea6e

vercel bot deployed to staging May 30, 2019 08:35 View deployment

patrickhulce reviewed May 30, 2019

View reviewed changes

lighthouse-core/report/html/report-styles.css Outdated Show resolved Hide resolved

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

f66136f

Co-Authored-By: Patrick Hulce <[email protected]>

vercel bot deployed to staging May 30, 2019 19:53 View deployment

googlebot added cla: no and removed cla: yes labels May 30, 2019

GoogleChrome deleted a comment from googlebot May 30, 2019

patrickhulce approved these changes May 30, 2019

View reviewed changes

connorjclark added 2 commits May 30, 2019 13:56

reportContainer

c08ce10

Merge branch 'rd-devtools-topbar' of github.com:GoogleChrome/lighthou…

bee3592

…se into rd-devtools-topbar

vercel bot deployed to staging May 30, 2019 20:59 View deployment

brendankenny approved these changes May 30, 2019

View reviewed changes

GoogleChrome deleted a comment from googlebot May 30, 2019

connorjclark merged commit 7671166 into master May 30, 2019

paulirish deleted the rd-devtools-topbar branch June 10, 2019 17:27

snyk-bot mentioned this pull request Mar 21, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

devtools: move topbar out of scrollable container #9077

devtools: move topbar out of scrollable container #9077

connorjclark commented May 29, 2019 •

edited

Loading

brendankenny commented May 29, 2019

connorjclark commented May 29, 2019

patrickhulce left a comment

patrickhulce May 30, 2019

patrickhulce May 30, 2019

patrickhulce May 30, 2019

connorjclark May 30, 2019 •

edited

Loading

patrickhulce May 30, 2019

connorjclark May 30, 2019

patrickhulce May 30, 2019

connorjclark May 30, 2019 •

edited

Loading

patrickhulce May 30, 2019

connorjclark May 30, 2019

patrickhulce commented May 30, 2019

connorjclark commented May 30, 2019

connorjclark commented May 30, 2019

brendankenny commented May 30, 2019

connorjclark commented May 30, 2019

exterkamp commented May 30, 2019

patrickhulce commented May 30, 2019

brendankenny commented May 30, 2019

connorjclark commented May 30, 2019 •

edited

Loading

patrickhulce left a comment

brendankenny left a comment

		reportFragment.appendChild(container);
		container.appendChild(headerContainer);

devtools: move topbar out of scrollable container #9077

devtools: move topbar out of scrollable container #9077

Conversation

connorjclark commented May 29, 2019 • edited Loading

brendankenny commented May 29, 2019

connorjclark commented May 29, 2019

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

connorjclark May 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

connorjclark May 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

patrickhulce commented May 30, 2019

connorjclark commented May 30, 2019

connorjclark commented May 30, 2019

brendankenny commented May 30, 2019

connorjclark commented May 30, 2019

exterkamp commented May 30, 2019

patrickhulce commented May 30, 2019

brendankenny commented May 30, 2019

connorjclark commented May 30, 2019 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

connorjclark commented May 29, 2019 •

edited

Loading

connorjclark May 30, 2019 •

edited

Loading

connorjclark May 30, 2019 •

edited

Loading

connorjclark commented May 30, 2019 •

edited

Loading