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): order gauges - default, pwa, plugins #8529

Merged
merged 3 commits into from
Apr 23, 2019
Merged

report(redesign): order gauges - default, pwa, plugins #8529

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68cd820
report(redesign): order gauges - mainstream, pwa, plugins
connorjclark Apr 22, 2019
74c14ce
test
connorjclark Apr 23, 2019
8c074fe
default, comment
connorjclark Apr 23, 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: 9 additions & 6 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 @@ -223,20 +223,23 @@ class ReportRenderer {
// }

if (scoreHeader) {
const defaultGauges = [];
const customGauges = [];
// Group gauges in this order: mainstream, pwa, plugins.
const mainstreamGauges = [];
Copy link
Member

Choose a reason for hiding this comment

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

can we still use "deault" (from default-config.js) or "core" for these?

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

const customGauges = []; // PWA.
const pluginGauges = [];
for (const category of report.reportCategories) {
const renderer = specificCategoryRenderers[category.id] || categoryRenderer;
const categoryGauge = renderer.renderScoreGauge(category, report.categoryGroups || {});

// Group gauges that aren't default at the end of the header
if (renderer.renderScoreGauge === categoryRenderer.renderScoreGauge) {
defaultGauges.push(categoryGauge);
if (Util.isPluginCategory(category.id)) {
Copy link
Member

Choose a reason for hiding this comment

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

Lol great Util function!

pluginGauges.push(categoryGauge);
} else if (renderer.renderScoreGauge === categoryRenderer.renderScoreGauge) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems like an interesting way to tell, I guess the point is "do we render this gauge exactly like the others or does the gauge look different"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i 100% don't understand renderer.renderScoreGauge === categoryRenderer.renderScoreGauge.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we weren't alone 😆 https://github.com/GoogleChrome/lighthouse/pull/6526/files#r234373117

Copy link
Member

Choose a reason for hiding this comment

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

if it's confusing, should keep the comment :) (or make it more informative!)

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

mainstreamGauges.push(categoryGauge);
} else {
customGauges.push(categoryGauge);
}
}
scoreHeader.append(...defaultGauges, ...customGauges);
scoreHeader.append(...mainstreamGauges, ...customGauges, ...pluginGauges);

const scoreScale = this._dom.cloneTemplate('#tmpl-lh-scorescale', this._templateContext);
const scoresContainer = this._dom.find('.lh-scores-container', headerContainer);
Expand Down
25 changes: 21 additions & 4 deletions 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 @@ -104,21 +104,38 @@ describe('ReportRenderer', () => {
assert.ok(header.querySelector('.lh-scores-container'), 'contains score container');
});

it('renders special score gauges after the mainstream ones', () => {
it('renders score gauges in this order: mainstream, pwa, plugins', () => {
const sampleResultsCopy = JSON.parse(JSON.stringify(sampleResults));
sampleResultsCopy.categories['lighthouse-plugin-someplugin'] = {
id: 'lighthouse-plugin-someplugin',
title: 'Some Plugin',
auditRefs: [],
};

const container = renderer._dom._document.body;
const output = renderer.renderReport(sampleResults, container);
const output = renderer.renderReport(sampleResultsCopy, container);

const indexOfFirstIrregularGauge = Array.from(output
const indexOfPwaGauge = Array.from(output
.querySelectorAll('.lh-scores-header > a[class*="lh-gauge"]')).findIndex(el => {
return el.matches('.lh-gauge--pwa__wrapper');
});

const indexOfPluginGauge = Array.from(output
.querySelectorAll('.lh-scores-header > a[class*="lh-gauge"]')).findIndex(el => {
return el.matches('.lh-gauge__wrapper--plugin');
});

assert.notEqual(-1, indexOfPwaGauge);
assert.notEqual(-1, indexOfPluginGauge);
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 also want to assert that indexOfPwaGauge is less than indexOfPluginGauge? and/or that they're the last two gauges?

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


const scoresHeaderElem = output.querySelector('.lh-scores-header');
for (let i = 0; i < scoresHeaderElem.children.length; i++) {
const gauge = scoresHeaderElem.children[i];

assert.ok(gauge.classList.contains('lh-gauge__wrapper'));
if (i >= indexOfFirstIrregularGauge) {
if (i >= indexOfPluginGauge) {
assert.ok(gauge.classList.contains('lh-gauge__wrapper--plugin'));
} else if (i >= indexOfPwaGauge) {
assert.ok(gauge.classList.contains('lh-gauge--pwa__wrapper'));
}
}
Expand Down