Skip to content
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.

Sign up for GitHub

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

Jump to bottom

report(redesign): add sticky scores header #8524

Merged
merged 23 commits into from
Apr 25, 2019
Merged

report(redesign): add sticky scores header #8524

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9f3268e
report(redesign): sticky header
connorjclark Apr 22, 2019
9552d04
stuck
connorjclark Apr 22, 2019
13467f1
no align items
connorjclark Apr 22, 2019
198441d
highlighter
connorjclark Apr 22, 2019
e271f89
comment
connorjclark Apr 22, 2019
039c2a9
padding bottom. resize is on window.
connorjclark Apr 22, 2019
06fe7b8
comment
connorjclark Apr 22, 2019
02c8970
.lh-sticky-header--stuck
connorjclark Apr 22, 2019
2a4c356
report ui features
connorjclark Apr 22, 2019
7455ec4
tests
connorjclark Apr 22, 2019
da82714
declarative
connorjclark Apr 22, 2019
a379435
visible
connorjclark Apr 22, 2019
fe023ee
htmlel
connorjclark Apr 22, 2019
e91c833
this.
connorjclark Apr 22, 2019
4a3159c
Merge remote-tracking branch 'origin/master' into report-ui-sticky-he…
connorjclark Apr 24, 2019
c99fbd7
fix jsdom
connorjclark Apr 24, 2019
0613ab7
add comment
connorjclark Apr 24, 2019
1c369d1
_renderScoreGauges
connorjclark Apr 24, 2019
23c5c2f
_updateStickyHeaderOnScroll
connorjclark Apr 24, 2019
4377ec4
ws
connorjclark Apr 24, 2019
1179d51
early return, delete comment
connorjclark Apr 25, 2019
557db06
var
connorjclark Apr 25, 2019
365f5e5
avoid unnecessary layout
connorjclark Apr 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions lighthouse-core/report/html/renderer/report-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,25 @@ 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');
Copy link
Member

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

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

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not cloned anymore

for (const gaugeWrapperEl of this._dom.findAll('.lh-gauge__wrapper', scoreHeader)) {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels pretty squicky to me :)

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

stickyHeader.appendChild(gaugeWrapperEl.cloneNode(true));
}

reportFragment.appendChild(stickyHeader);
}

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

Expand Down
42 changes: 42 additions & 0 deletions lighthouse-core/report/html/renderer/report-ui-features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class ReportUIFeatures {
this.onKeyDown = this.onKeyDown.bind(this);
this.printShortCutDetect = this.printShortCutDetect.bind(this);
this.onChevronClick = this.onChevronClick.bind(this);
this._handleStickyHeader = this._handleStickyHeader.bind(this);
}

/**
Expand All @@ -65,6 +66,11 @@ class ReportUIFeatures {
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup that was it, fixed.

if (window.addEventListener) {
window.addEventListener('resize', this._handleStickyHeader);
}
}

/**
Expand Down Expand Up @@ -380,6 +386,42 @@ class ReportUIFeatures {
this._document.body.removeChild(a);
setTimeout(_ => URL.revokeObjectURL(href), 500);
}

_handleStickyHeader() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@connorjclark connorjclark Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_handleStickyHeaderVisibilityAndHighlighterOnScrollAndOnResize? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sold, to _updateStickyHeaderOnScroll

const topbarEl = this._document.querySelector('.lh-topbar');
const scoreScaleEl = this._document.querySelector('.lh-scorescale');
const stickyHeaderEl = this._document.querySelector('.lh-sticky-header');
const highlightEl = this._document.querySelector('.lh-highlighter');
if (!topbarEl || !scoreScaleEl || !stickyHeaderEl || !highlightEl) return;

// 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('lh-sticky-header--stuck', showStickyHeader);

// Highlight mini gauge when section is in view.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

early return if showStickyHeader === false?

// 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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, why not Infinity? :)

const categoryEls = this._document.querySelectorAll('.lh-category');
for (const [index, categoryEl] of Object.entries(categoryEls)) {
// Normalize to middle of viewport.
const distanceToMiddle = categoryEl.getBoundingClientRect().top - window.innerHeight / 2;
// Closest negative distance to zero wins.
if (distanceToMiddle < 0 && highlightIndexDistance > distanceToMiddle) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

highlightIndex = parseInt(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
// this comment when that is done.
const gaugeToHighlight = stickyHeaderEl.querySelectorAll('.lh-gauge__wrapper')[highlightIndex];
// @ts-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are we ignoring?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be HTMLElement

highlightEl.style.left = gaugeToHighlight.getBoundingClientRect().left + 'px';
}
}

if (typeof module !== 'undefined' && module.exports) {
Expand Down
6 changes: 5 additions & 1 deletion lighthouse-core/report/html/report-styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,8 @@
--color-fail: var(--color-red);
--color-pass-secondary: var(--color-green-700);
--color-pass: var(--color-green);
--color-sticky-header-bg: var(--color-white);
--color-highlighter-bg: var(--color-black);

--plugin-icon-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="24px" height="24px" viewBox="0 0 24 24" fill="%23757575"><path d="M0 0h24v24H0z" fill="none"/><path d="M20.5 11H19V7c0-1.1-.9-2-2-2h-4V3.5C13 2.12 11.88 1 10.5 1S8 2.12 8 3.5V5H4c-1.1 0-1.99.9-1.99 2v3.8H3.5c1.49 0 2.7 1.21 2.7 2.7s-1.21 2.7-2.7 2.7H2V20c0 1.1.9 2 2 2h3.8v-1.5c0-1.49 1.21-2.7 2.7-2.7 1.49 0 2.7 1.21 2.7 2.7V22H17c1.1 0 2-.9 2-2v-4h1.5c1.38 0 2.5-1.12 2.5-2.5S21.88 11 20.5 11z"/></svg>');
--logo-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="192" height="192"><g fill="none" fill-rule="evenodd"><path d="M0 0h192v192H0z"/><path d="M67.705 179.352l2.603-20.82 49.335-16.39 4.652 37.21A87.893 87.893 0 0 1 96 184a87.893 87.893 0 0 1-28.295-4.648zM52.44 172.48C25.894 157.328 8 128.754 8 96 8 47.399 47.399 8 96 8s88 39.399 88 88c0 32.754-17.894 61.328-44.44 76.48L130 96h6V80h-8V48L96 28 64 48v32h-8v16h6l-9.56 76.48zM113.875 96l2.882 23.05-43.318 14.433L78.125 96h35.75zM80 80V56.868l16-10 16 10V80H80z" fill="%23000000"/></g></svg>');
Expand Down Expand Up @@ -838,7 +840,9 @@
}

.lh-scores-header .lh-gauge__wrapper,
.lh-scores-header .lh-gauge--pwa__wrapper {
.lh-scores-header .lh-gauge--pwa__wrapper,
.lh-sticky-header .lh-gauge__wrapper,
.lh-sticky-header .lh-gauge--pwa__wrapper {
width: var(--score-container-width);
}

Expand Down
43 changes: 41 additions & 2 deletions lighthouse-core/report/html/templates.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,49 @@
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>
<div class="lh-scores-wrapper">
<div class="lh-scores-container">
</div>
<div class="lh-scores-container"></div>
</div>
</template>

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/report/html/renderer/report-renderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a comment for where this multiplier comes from

// no fireworks
assert.ok(output.querySelector('.score100') === null, 'has no fireworks treatment');
});
Expand Down