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.

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: New audit group clump for audits with warnings #6974

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions lighthouse-core/lib/i18n/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -1079,6 +1079,10 @@
"message": "Values are estimated and may vary.",
"description": "Disclaimer shown to users below the metric values (First Contentful Paint, Time to Interactive, etc) to warn them that the numbers they see will likely change slightly the next time they run Lighthouse."
},
"lighthouse-core/report/html/renderer/util.js | warningAuditsGroupTitle": {
"message": "Audits with warnings",
"description": "Section heading shown above a list of audits that contain warnings."
},
"lighthouse-core/report/html/renderer/util.js | warningHeader": {
"message": "Warnings: ",
"description": "This label is shown above a bulleted list of warnings. It is shown directly below an audit that produced warnings. Warnings describe situations the user should be aware of, as Lighthouse was unable to complete all the work required on this audit. For example, The 'Unable to decode image (biglogo.jpg)' warning may show up below an image encoding audit."
Expand Down
59 changes: 48 additions & 11 deletions lighthouse-core/report/html/renderer/category-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
/** @typedef {import('./report-renderer.js')} ReportRenderer */
/** @typedef {import('./details-renderer.js')} DetailsRenderer */
/** @typedef {import('./util.js')} Util */
/** @typedef {'failed'|'manual'|'passed'|'notApplicable'} TopLevelClumpId */
/** @typedef {'failed'|'manual'|'warning'|'passed'|'notApplicable'} TopLevelClumpId */

class CategoryRenderer {
/**
Expand All @@ -46,19 +46,34 @@ class CategoryRenderer {
get _clumpDisplayInfo() {
return {
failed: {
title: '',
className: 'lh-clump--failed',
expandable: false,
expandByDefault: false,
Copy link
Member

Choose a reason for hiding this comment

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

arg, sorry, I was hoping you'd wait for #6930. I think a lot of this file can be deleted. I'm not sure it even makes sense to have clumps anymore since groups and clumps won't be at all alike anymore.

Copy link
Collaborator Author

@connorjclark connorjclark Jan 10, 2019

Choose a reason for hiding this comment

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

ok, i'll wait for that. third time's a charm

},
warning: {
title: Util.UIStrings.warningAuditsGroupTitle,
className: 'lh-clump--warning',
expandable: true,
expandByDefault: true,
},
manual: {
title: Util.UIStrings.manualAuditsGroupTitle,
className: 'lh-clump--manual',
expandable: true,
expandByDefault: false,
},
passed: {
title: Util.UIStrings.passedAuditsGroupTitle,
className: 'lh-clump--passed',
expandable: true,
expandByDefault: false,
},
notApplicable: {
title: Util.UIStrings.notApplicableAuditsGroupTitle,
className: 'lh-clump--notapplicable',
expandable: true,
expandByDefault: false,
},
};
}
Expand Down Expand Up @@ -183,7 +198,7 @@ class CategoryRenderer {
* Renders the group container for a group of audits. Individual audit elements can be added
* directly to the returned element.
* @param {LH.Result.ReportGroup} group
* @param {{expandable: boolean, itemCount?: number}} opts
* @param {{expandable: boolean, expandByDefault: boolean, itemCount?: number}} opts
* @return {Element}
*/
renderAuditGroup(group, opts) {
Expand All @@ -196,6 +211,9 @@ class CategoryRenderer {
if (expandable) {
const chevronEl = summaryInnerEl.appendChild(this._createChevron());
chevronEl.title = Util.UIStrings.auditGroupExpandTooltip;
if (opts.expandByDefault) {
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 this is ripe for some cleanup like our boolean traps over in page load.

WDYT about an enum instead of the two booleans type ExpansionBehavior = 'open-permanent' | 'closed-expandable' | 'open-expandable' or something like that?

groupEl.setAttribute('open', '');
}
}

if (group.description) {
Expand All @@ -217,7 +235,7 @@ class CategoryRenderer {
* array of audit and audit-group elements.
* @param {Array<LH.ReportResult.AuditRef>} auditRefs
* @param {Object<string, LH.Result.ReportGroup>} groupDefinitions
* @param {{expandable: boolean}} opts
* @param {{expandable: boolean, expandByDefault: boolean}} opts
* @return {Array<Element>}
*/
_renderGroupedAudits(auditRefs, groupDefinitions, opts) {
Expand Down Expand Up @@ -272,14 +290,17 @@ class CategoryRenderer {
*/
renderUnexpandableClump(auditRefs, groupDefinitions) {
const clumpElement = this.dom.createElement('div');
const elements = this._renderGroupedAudits(auditRefs, groupDefinitions, {expandable: false});
const elements = this._renderGroupedAudits(auditRefs, groupDefinitions, {
expandable: false,
expandByDefault: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this one is a bit confusing since won't it be expanded by default? how else would you expand it? maybe this value shouldn't matter when expandable is false

});
elements.forEach(elem => clumpElement.appendChild(elem));
return clumpElement;
}

/**
* Renders a clump (a grouping of groups), under a status of failed, manual,
* passed, or notApplicable. The result ends up something like:
* warning, passed, or notApplicable. The result ends up something like:
*
* clump (e.g. 'failed')
* ├── audit 1 (w/o group)
Expand All @@ -298,20 +319,25 @@ class CategoryRenderer {
* @return {Element}
*/
renderClump(clumpId, {auditRefs, groupDefinitions, description}) {
if (clumpId === 'failed') {
const clumpInfo = this._clumpDisplayInfo[clumpId];
const expandable = clumpInfo.expandable;
const expandByDefault = clumpInfo.expandByDefault;

if (!expandable) {
// Failed audit clump is always expanded and not nested in an lh-audit-group.
Copy link
Collaborator

Choose a reason for hiding this comment

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

update comment and names now then?

const failedElem = this.renderUnexpandableClump(auditRefs, groupDefinitions);
failedElem.classList.add('lh-clump', this._clumpDisplayInfo.failed.className);
return failedElem;
}

const expandable = true;
const elements = this._renderGroupedAudits(auditRefs, groupDefinitions, {expandable});
const elements = this._renderGroupedAudits(auditRefs, groupDefinitions, {
expandable,
expandByDefault,
});

const clumpInfo = this._clumpDisplayInfo[clumpId];
// TODO: renderAuditGroup shouldn't be used to render a clump (since it *contains* audit groups).
const groupDef = {title: clumpInfo.title, description};
const opts = {expandable, itemCount: auditRefs.length};
const opts = {expandable, expandByDefault, itemCount: auditRefs.length};
const clumpElem = this.renderAuditGroup(groupDef, opts);
clumpElem.classList.add('lh-clump', clumpInfo.className);

Expand Down Expand Up @@ -363,6 +389,14 @@ class CategoryRenderer {
return tmpl;
}

/**
* @param {LH.ReportResult.AuditRef} audit
* @return {boolean}
*/
_auditHasWarning(audit) {
return Boolean(audit.result.warnings && audit.result.warnings.length);
}

/**
* Returns the id of the top-level clump to put this audit in.
* @param {LH.ReportResult.AuditRef} auditRef
Expand All @@ -374,7 +408,9 @@ class CategoryRenderer {
return scoreDisplayMode;
}

if (Util.showAsPassed(auditRef.result)) {
if (this._auditHasWarning(auditRef)) {
return 'warning';
} else if (Util.showAsPassed(auditRef.result)) {
return 'passed';
} else {
return 'failed';
Expand All @@ -395,6 +431,7 @@ class CategoryRenderer {
/** @type {Map<TopLevelClumpId, Array<LH.ReportResult.AuditRef>>} */
const clumps = new Map();
clumps.set('failed', []);
clumps.set('warning', []);
clumps.set('manual', []);
clumps.set('passed', []);
clumps.set('notApplicable', []);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,10 @@ class PerformanceCategoryRenderer extends CategoryRenderer {

// Metrics
const metricAudits = category.auditRefs.filter(audit => audit.group === 'metrics');
const metricAuditsEl = this.renderAuditGroup(groups.metrics, {expandable: false});
const metricAuditsEl = this.renderAuditGroup(groups.metrics, {
expandable: false,
expandByDefault: false,
});

const keyMetrics = metricAudits.filter(a => a.weight >= 3);
const otherMetrics = metricAudits.filter(a => a.weight < 3);
Expand Down Expand Up @@ -176,7 +179,10 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
const wastedMsValues = opportunityAudits.map(audit => this._getWastedMs(audit));
const maxWaste = Math.max(...wastedMsValues);
const scale = Math.max(Math.ceil(maxWaste / 1000) * 1000, minimumScale);
const groupEl = this.renderAuditGroup(groups['load-opportunities'], {expandable: false});
const groupEl = this.renderAuditGroup(groups['load-opportunities'], {
expandable: false,
expandByDefault: false,
});
const tmpl = this.dom.cloneTemplate('#tmpl-lh-opportunity-header', this.templateContext);

this.dom.find('.lh-load-opportunity__col--one', tmpl).textContent =
Expand All @@ -202,7 +208,10 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
});

if (diagnosticAudits.length) {
const groupEl = this.renderAuditGroup(groups['diagnostics'], {expandable: false});
const groupEl = this.renderAuditGroup(groups['diagnostics'], {
expandable: false,
expandByDefault: false,
});
diagnosticAudits.forEach((item, i) => groupEl.appendChild(this.renderAudit(item, i)));
groupEl.classList.add('lh-audit-group--diagnostics');
element.appendChild(groupEl);
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-core/report/html/renderer/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,8 @@ Util.UIStrings = {
warningHeader: 'Warnings: ',
/** The tooltip text on an expandable chevron icon. Clicking the icon expands a section to reveal a list of audit results that was hidden by default. */
auditGroupExpandTooltip: 'Show audits',
/** Section heading shown above a list of audits that contain warnings. */
warningAuditsGroupTitle: 'Audits with warnings',
/** Section heading shown above a list of audits that are passing. 'Passed' here refers to a passing grade. This section is collapsed by default, as the user should be focusing on the failed audits instead. Users can click this heading to reveal the list. */
passedAuditsGroupTitle: 'Passed audits',
/** Section heading shown above a list of audits that do not apply to the page. For example, if an audit is 'Are images optimized?', but the page has no images on it, the audit will be marked as not applicable. This is neither passing or failing. This section is collapsed by default, as the user should be focusing on the failed audits instead. Users can click this heading to reveal the list. */
Expand Down
4 changes: 4 additions & 0 deletions lighthouse-core/report/html/report-styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,10 @@
vertical-align: middle;
}

.lh-clump--warning > summary .lh-audit-group__header::before {
content: '';
background-image: var(--search-icon-url);
}
.lh-clump--manual > summary .lh-audit-group__header::before {
content: '';
background-image: var(--search-icon-url);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,24 @@ describe('CategoryRenderer', () => {
assert.ok(warningEl.textContent.includes(auditResult.warnings[1]), '2nd warning provided');
});

it('expands warning audit group', () => {
const category = sampleResults.reportCategories.find(c => c.id === 'pwa');
const prevAuditRefs = category.auditRefs;
const auditWithWarning = prevAuditRefs[0];
auditWithWarning.result = Object.assign({}, category.auditRefs[0].result,
{warnings: ['Some warning']});
category.auditRefs = [auditWithWarning];

const auditDOM = renderer.render(category, sampleResults.categoryGroups);
auditDOM.querySelectorAll('.lh-clump--warning, .lh-clump--warning .lh-audit-group')
.forEach(el => {
const isExpanded = el.hasAttribute('open');
assert.ok(isExpanded, 'Warning audit group should be expanded by default');
});

category.auditRefs = prevAuditRefs;
});

it('renders a category', () => {
const category = sampleResults.reportCategories.find(c => c.id === 'pwa');
const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
Expand Down Expand Up @@ -307,15 +325,17 @@ describe('CategoryRenderer', () => {
});
});

describe('clumping passed/failed/manual', () => {
describe('clumping passed/warning/failed/manual', () => {
it('separates audits in the DOM', () => {
const category = sampleResults.reportCategories.find(c => c.id === 'pwa');
const elem = renderer.render(category, sampleResults.categoryGroups);
const passedAudits = elem.querySelectorAll('.lh-clump--passed .lh-audit');
const warningAudits = elem.querySelectorAll('.lh-clump--warning .lh-audit');
const failedAudits = elem.querySelectorAll('.lh-clump--failed .lh-audit');
const manualAudits = elem.querySelectorAll('.lh-clump--manual .lh-audit');

assert.equal(passedAudits.length, 4);
assert.equal(passedAudits.length, 3);
assert.equal(warningAudits.length, 1);
assert.equal(failedAudits.length, 8);
assert.equal(manualAudits.length, 3);
});
Expand All @@ -326,10 +346,12 @@ describe('CategoryRenderer', () => {
category.auditRefs.forEach(audit => audit.result.score = 0);
const elem = renderer.render(category, sampleResults.categoryGroups);
const passedAudits = elem.querySelectorAll('.lh-clump--passed .lh-audit');
const warningAudits = elem.querySelectorAll('.lh-clump--warning .lh-audit');
const failedAudits = elem.querySelectorAll('.lh-clump--failed .lh-audit');

assert.equal(passedAudits.length, 0);
assert.equal(failedAudits.length, 12);
assert.equal(warningAudits.length, 1);
assert.equal(failedAudits.length, 11);
});
});

Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/test/results/sample_v2.json
Original file line number Diff line number Diff line change
Expand Up @@ -4343,6 +4343,7 @@
"scorescaleLabel": "Score scale:",
"toplevelWarningsMessage": "There were issues affecting this run of Lighthouse:",
"varianceDisclaimer": "Values are estimated and may vary.",
"warningAuditsGroupTitle": "Audits with warnings",
"warningHeader": "Warnings: "
},
"icuMessagePaths": {
Expand Down
3 changes: 3 additions & 0 deletions proto/lighthouse-result.proto
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,9 @@ message I18n {

// The title of the lab data performance category
string lab_data_title = 16;

// The heading that is shown above a list of audits that have warnings
string warning_audits_group_title = 17;
}

// The message holding all formatted strings
Expand Down
1 change: 1 addition & 0 deletions proto/sample_v2_round_trip.json
Original file line number Diff line number Diff line change
Expand Up @@ -3286,6 +3286,7 @@
"scorescaleLabel": "Score scale:",
"toplevelWarningsMessage": "There were issues affecting this run of Lighthouse:",
"varianceDisclaimer": "Values are estimated and may vary.",
"warningAuditsGroupTitle": "Audits with warnings",
"warningHeader": "Warnings: "
}
},
Expand Down