-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Changes from all commits
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 |
---|---|---|
|
@@ -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 { | ||
/** | ||
|
@@ -46,19 +46,34 @@ class CategoryRenderer { | |
get _clumpDisplayInfo() { | ||
return { | ||
failed: { | ||
title: '', | ||
className: 'lh-clump--failed', | ||
expandable: false, | ||
expandByDefault: false, | ||
}, | ||
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, | ||
}, | ||
}; | ||
} | ||
|
@@ -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) { | ||
|
@@ -196,6 +211,9 @@ class CategoryRenderer { | |
if (expandable) { | ||
const chevronEl = summaryInnerEl.appendChild(this._createChevron()); | ||
chevronEl.title = Util.UIStrings.auditGroupExpandTooltip; | ||
if (opts.expandByDefault) { | ||
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. 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 |
||
groupEl.setAttribute('open', ''); | ||
} | ||
} | ||
|
||
if (group.description) { | ||
|
@@ -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) { | ||
|
@@ -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, | ||
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 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 |
||
}); | ||
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) | ||
|
@@ -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. | ||
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. 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); | ||
|
||
|
@@ -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 | ||
|
@@ -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'; | ||
|
@@ -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', []); | ||
|
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.
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.
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, i'll wait for that. third time's a charm