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: snapshot/timespan sticky header #13617

Closed
wants to merge 6 commits into from
Closed

report: snapshot/timespan sticky header #13617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b78b36
report: new sticky header
adamraine Feb 2, 2022
df42463
highlight
adamraine Feb 2, 2022
77f0158
narrow
adamraine Feb 2, 2022
3d91621
sample
adamraine Feb 2, 2022
6f61f76
update script
adamraine Feb 2, 2022
3d583e1
comment
adamraine Feb 2, 2022
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
6 changes: 6 additions & 0 deletions build/build-sample-reports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ const flowResult = readJson(
`${LH_ROOT}/lighthouse-core/test/fixtures/fraggle-rock/reports/sample-flow-result.json`
);

/** @type {LH.Result} */
const snapshotLhr = readJson(
`${LH_ROOT}/lighthouse-core/test/fixtures/fraggle-rock/reports/sample-snapshot-lhr.json`
adamraine marked this conversation as resolved.
Show resolved Hide resolved
);

const DIST = path.join(LH_ROOT, 'dist');

(async function() {
Expand All @@ -36,6 +41,7 @@ const DIST = path.join(LH_ROOT, 'dist');
'xl-accented': swapLocale(lhr, 'en-XL').lhr,
'error': errorLhr,
'single-category': tweakLhrForPsi(lhr),
'snapshot': snapshotLhr,
};

// Generate and write reports
Expand Down
35 changes: 35 additions & 0 deletions lighthouse-core/scripts/update-snapshot-sample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* @license Copyright 2022 The Lighthouse Authors. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

import fs from 'fs';

import puppeteer from 'puppeteer';

import {LH_ROOT} from '../../root.js';
import {snapshot} from '../fraggle-rock/api.js';

(async () => {
const browser = await puppeteer.launch({
ignoreDefaultArgs: ['--enable-automation'],
executablePath: process.env.CHROME_PATH,
headless: false,
});
const page = await browser.newPage();
await page.goto('https://example.com/');

const result = await snapshot({page});
if (!result || typeof result.report !== 'string') {
throw new Error('LHR string not found');
}

fs.writeFileSync(
`${LH_ROOT}/lighthouse-core/test/fixtures/fraggle-rock/reports/sample-snapshot-lhr.json`,
result.report
);

await browser.close();
})();
2,468 changes: 2,468 additions & 0 deletions lighthouse-core/test/fixtures/fraggle-rock/reports/sample-snapshot-lhr.json
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
"update:sample-artifacts": "node lighthouse-core/scripts/update-report-fixtures.js",
"update:sample-json": "yarn i18n:collect-strings && node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --config-path=./lighthouse-core/test/results/sample-config.js --output=json --output-path=./lighthouse-core/test/results/sample_v2.json && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing",
"update:flow-sample-json": "node ./lighthouse-core/scripts/update-flow-fixtures.js",
"update:snapshot-sample-json": "node ./lighthouse-core/scripts/update-snapshot-sample.js",
"update:test-devtools": "bash lighthouse-core/test/chromium-web-tests/test-locally.sh --reset-results",
"test-devtools": "bash lighthouse-core/test/chromium-web-tests/test-locally.sh",
"open-devtools": "bash lighthouse-core/scripts/open-devtools.sh",
Expand Down
10 changes: 6 additions & 4 deletions report/assets/styles.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 19 additions & 13 deletions report/assets/templates.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,6 @@
justify-content: center;
background-color: var(--sticky-header-background-color);
border-bottom: 1px solid var(--color-gray-200);
padding-top: var(--score-container-padding);
padding-bottom: 4px;
z-index: 1;
pointer-events: none;
}
Expand All @@ -189,18 +187,26 @@
animation: none;
}

.lh-sticky-header .lh-gauge__label {
display: none;
.lh-sticky-header .lh-gauge__label,
.lh-sticky-header .lh-fraction__label {
margin-top: 0;
margin-left: 10px;
}

.lh-highlighter {
width: var(--gauge-wrapper-width);
height: 1px;
background-color: var(--highlighter-background-color);
/* Position at bottom of first gauge in sticky header. */
position: absolute;
grid-column: 1;
bottom: -1px;
@media screen and (max-width: 600px) {
.lh-sticky-header .lh-gauge__label,
.lh-sticky-header .lh-fraction__label {
display: none;
}
}

.lh-sticky-header .lh-gauge__wrapper,
.lh-sticky-header .lh-fraction__wrapper {
border-bottom: 1px transparent solid;
}

.lh-gauge--highlight {
border-color: var(--highlighter-background-color) !important;
}

.lh-gauge__wrapper:first-of-type {
Expand Down Expand Up @@ -541,8 +547,8 @@
<circle class="lh-gauge-base" r="56" cx="60" cy="60" stroke-width="8"></circle>
<circle class="lh-gauge-arc" r="56" cx="60" cy="60" stroke-width="8"></circle>
</svg>
<div class="lh-gauge__percentage"></div>
</div>
<div class="lh-gauge__percentage"></div>
<!-- TODO: should likely be an h2 -->
<div class="lh-gauge__label"></div>
</a>
Expand Down
8 changes: 4 additions & 4 deletions report/renderer/components.js
View file
Open in desktop

Large diffs are not rendered by default.

16 changes: 6 additions & 10 deletions report/renderer/topbar-features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ export class TopbarFeatures {
this.categoriesEl; // eslint-disable-line no-unused-expressions
/** @type {HTMLElement?} */
this.stickyHeaderEl; // eslint-disable-line no-unused-expressions
/** @type {HTMLElement} */
this.highlightEl; // eslint-disable-line no-unused-expressions
this.onDropDownMenuClick = this.onDropDownMenuClick.bind(this);
this.onKeyUp = this.onKeyUp.bind(this);
this.onCopy = this.onCopy.bind(this);
Expand Down Expand Up @@ -266,9 +264,6 @@ export class TopbarFeatures {
return;
}

// Highlighter will be absolutely positioned at first gauge, then transformed on scroll.
this.highlightEl = this._dom.createChildOf(this.stickyHeaderEl, 'div', 'lh-highlighter');

// Update sticky header visibility and highlight when page scrolls/resizes.
const scrollParent = this._getScrollParent(
this._dom.find('.lh-container', this._dom.rootEl));
Expand Down Expand Up @@ -302,13 +297,14 @@ export class TopbarFeatures {
categoriesAboveTheMiddle.length > 0 ? categoriesAboveTheMiddle.length - 1 : 0;

// Category order matches gauge order in sticky header.
const gaugeWrapperEls = this.stickyHeaderEl.querySelectorAll('.lh-gauge__wrapper');
const gaugeWrapperEls =
this.stickyHeaderEl.querySelectorAll('.lh-gauge__wrapper, .lh-fraction__wrapper');
const gaugeToHighlight = gaugeWrapperEls[highlightIndex];
const origin = gaugeWrapperEls[0].getBoundingClientRect().left;
const offset = gaugeToHighlight.getBoundingClientRect().left - origin;
for (const gauge of gaugeWrapperEls) {
gauge.classList.remove('lh-gauge--highlight');
}
gaugeToHighlight.classList.add('lh-gauge--highlight');

// Mutate at end to avoid layout thrashing.
this.highlightEl.style.transform = `translate(${offset}px)`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The above comment applied to this line. Is there not layout thrashing with these changes? I guess not because you made CSS changes to replace the manual transform setting?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you confirm w/ the performance panel that scrolling doesn't result in tons of style recalc?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is some small style updates, but those existed before this change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Paul reminded me... before, this highlighter was animated between these states. Now that's lost.

idk if we care tbh

Copy link
Collaborator

Choose a reason for hiding this comment

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

also we noticed that clicking the gauges at the top doesnt always update highlighter correctly, viewport size seems to make it vary

Copy link
Member Author

Choose a reason for hiding this comment

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

The highlight is based on whichever category is in the center of the viewport. For snapshot, performance category can be so small that it will never be in the center.

Honestly, one solution could be to make Accessibility the first category for snapshot reports. Performance isn't important enough to be first for snapshot reports IMO.

this.stickyHeaderEl.classList.toggle('lh-sticky-header--visible', showStickyHeader);
}
}