-
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
report(redesign): add sticky scores header #8524
Changes from 6 commits
9f3268e
9552d04
13467f1
198441d
e271f89
039c2a9
06fe7b8
02c8970
2a4c356
7455ec4
da82714
a379435
fe023ee
e91c833
4a3159c
c99fbd7
0613ab7
1c369d1
23c5c2f
4377ec4
1179d51
557db06
365f5e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -247,10 +247,24 @@ class ReportRenderer { | |
reportSection.appendChild(this._renderReportFooter(report)); | ||
|
||
const reportFragment = this._dom.createFragment(); | ||
|
||
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'); | ||
this._dom.createChildOf(stickyHeader, 'div', 'lh-highlighter'); | ||
|
||
for (const gaugeWrapperEl of this._dom.findAll('.lh-gauge__wrapper', scoreHeader)) { | ||
patrickhulce marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this feels pretty squicky to me :) Can we make a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
const gaugeWrapperElClone = gaugeWrapperEl.cloneNode(true); | ||
stickyHeader.appendChild(gaugeWrapperElClone.cloneNode(true)); | ||
} | ||
|
||
reportFragment.appendChild(stickyHeader); | ||
} | ||
|
||
reportFragment.appendChild(headerContainer); | ||
reportFragment.appendChild(container); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,10 +144,85 @@ | |
position: relative; | ||
width: 100%; | ||
} | ||
|
||
.lh-sticky-header { | ||
--gauge-circle-size: 36px; | ||
--plugin-badge-size: 18px; | ||
--plugin-icon-size: 75%; | ||
--score-container-width: 60px; | ||
--score-number-font-size: 13px; | ||
position: fixed; | ||
left: 0; | ||
right: 0; | ||
top: var(--topbar-height); | ||
font-weight: 700; | ||
display: flex; | ||
justify-content: center; | ||
background-color: var(--color-sticky-header-bg); | ||
border-bottom: 1px solid var(--color-black-200); | ||
padding-top: var(--score-container-padding); | ||
padding-bottom: 8px; | ||
z-index: 1000; | ||
pointer-events: none; | ||
opacity: 0; | ||
} | ||
|
||
.lh-sticky-header.stuck { | ||
pointer-events: auto; | ||
opacity: 1; | ||
patrickhulce marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
.lh-sticky-header .lh-gauge__label { | ||
display: none; | ||
} | ||
|
||
.lh-highlighter { | ||
width: var(--score-container-width); | ||
height: 1px; | ||
background: var(--color-highlighter-bg); | ||
position: absolute; | ||
bottom: -1px; | ||
left: 0; | ||
} | ||
</style> | ||
<script> | ||
function handleStickyHeader() { | ||
patrickhulce marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const topbarEl = document.querySelector('.lh-topbar'); | ||
const scoreScaleEl = document.querySelector('.lh-scorescale'); | ||
const stickyHeaderEl = document.querySelector('.lh-sticky-header'); | ||
const highlightEl = document.querySelector('.lh-highlighter'); | ||
|
||
// 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we use |
||
|
||
// Highlight mini gauge when section is in view. | ||
// 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; | ||
for (const [index, categoryEl] of Object.entries(document.querySelectorAll('.lh-category'))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just confirming you are correct in your next comment - this code will work as expected when the gauge order is as expected. |
||
// Normalize to middle of viewport. | ||
const distanceToMiddle = categoryEl.getBoundingClientRect().top - window.innerHeight / 2; | ||
// Closest negative distance to zero wins. | ||
if (distanceToMiddle < 0 && highlightIndexDistance > distanceToMiddle) { | ||
highlightIndex = index; | ||
highlightIndexDistance = -distanceToMiddle; | ||
} | ||
} | ||
|
||
// Category order matches gauge order in sticky header. | ||
// TODO(hoten): not 100% true yet, need to order gauges like: core, pwa, plugins. Remove | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, I see :) historically comments without a direct issue link tend to be forgotten, is that PR like the next one though? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a task entry in the report-redesign issue for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah of course! |
||
// this comment when that is done. | ||
const gaugeToHighlight = stickyHeaderEl.querySelectorAll('.lh-gauge__wrapper')[highlightIndex]; | ||
highlightEl.style.left = gaugeToHighlight.getBoundingClientRect().left + 'px'; | ||
} | ||
document.addEventListener('scroll', handleStickyHeader); | ||
window.addEventListener('resize', handleStickyHeader); | ||
</script> | ||
<div class="lh-scores-wrapper"> | ||
<div class="lh-scores-container"> | ||
</div> | ||
<div class="lh-scores-container"></div> | ||
</div> | ||
</template> | ||
|
||
|
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.
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
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.
imo that doesn't seem like a useful test, since these gauges are created with the same function.
jsdom/jsdom#1422 (comment) this makes me think it should be possible to fake it
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.
ok it'd be way too much faking, basically all the
getBoundingClientRect
needs to be mocked