-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
devtools: enable sticky header, top bar, and report ui features #9023
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exciting times ahead for devtools :D
|
||
if (this._dom.isDevTools()) { | ||
// Not ready for DevTools. | ||
// Saving as HTML save the entire AuditsPanel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm it's not super clear what's being enabled in DT and what's not since the comments sorta line up with what's in the else
.
not sure how to fix this maybe it's just moving to have a comment for each line explaining why it can't be enabled and/or when it might be moved out of the else
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure - ...does not work.
is not a good comment :)
@@ -260,6 +261,10 @@ | |||
--audit-group-indent: 16px; | |||
--audit-indent: 16px; | |||
--expandable-indent: 16px; | |||
--gauge-circle-size-big: 96px; | |||
--gauge-circle-size: 72px; | |||
/* Consider offset of the DevTools UI. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these feels slightly magical is there anyway we can link to how to compute this number in the future? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I actually just increased the offset value until the topbar was 100% visible and there was no gap between the DevTools UI + the topbar.
Think I should look into computing the offset at runtime, and somehow injecting the CSS variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh haha, nah maybe just make a note of "this is the height of the devtools UI as eyeballed by a developer in styles panel" or something? 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
|
||
if (scoreHeader && !this._dom.isDevTools()) { | ||
if (scoreHeader) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we combine these if (scoreHeader)
blocks now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
// We can't rely on listening to the window resize event for DevTools, so we first | ||
// attempt the new ResizeObserver web platform feature. It has poor cross browser | ||
// support, so we should check that it's supported. However, there are some performance | ||
// issues with using window.ResizeObserver - it updates much more often than the 'resize' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using the ResizeObserver in the deploy report (It's disabled outside devtools but if you revert that bit), and add some logging to the callback:
// Show sticky header when the score scale begins to go underneath the topbar.
const topbarBottom = this.topbarEl.getBoundingClientRect().bottom;
const scoreScaleTop = this.scoreScaleEl.getBoundingClientRect().top;
const showStickyHeader = topbarBottom >= scoreScaleTop;
// add this line
console.log(topbarBottom, scoreScaleTop, showStickyHeader);
You see that the position of our topbar
is jumping all over the place!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we should just disable it outside DevTools for now, luckily it works there perfectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last things, otherwise LGTM
} | ||
|
||
/* Disable the gauge arc animation for the sticky header, so toggling display: none | ||
does not play the animation. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
evidence number 25 that these different uses of the score gauges should share less than they do :)
// support, so we should check that it's supported. However, there are some performance | ||
// issues with using window.ResizeObserver - it updates much more often than the 'resize' | ||
// event fires, and the experience is choppy in LH. For now, limit use to just DevTools, | ||
// which doesn't seem affected for some reason. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since in theory ResizeObserver
is more efficient and the future path to monitoring resizes, what about
// Use ResizeObserver where available. TODO: there is an issue with incorrect position numbers and, as a result, performance issues due to layout thrashing. See https://github.com/GoogleChrome/lighthouse/pull/9023/files#r288822287 for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you also suggesting removing the devtools check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you also suggesting removing the devtools check?
whoops, no, good point. I like your version
there's a type check error...the type definition isn't working? It should be declared on window... |
tsconfig for viewer was wrong. Compiler option |
friggen flakes... |
LH changes for #9019.