-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 clump for audits with warnings (#5327) #6989
Changes from 1 commit
bb6b1aa
d660748
fb8834b
4dcd6dc
3737a96
3c322af
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'|'warning'|'manual'|'passed'|'notApplicable'} TopLevelClumpId */ | ||
|
||
class CategoryRenderer { | ||
/** | ||
|
@@ -45,6 +45,7 @@ class CategoryRenderer { | |
*/ | ||
get _clumpTitles() { | ||
return { | ||
warning: Util.UIStrings.warningAuditsGroupTitle, | ||
manual: Util.UIStrings.manualAuditsGroupTitle, | ||
passed: Util.UIStrings.passedAuditsGroupTitle, | ||
notApplicable: Util.UIStrings.notApplicableAuditsGroupTitle, | ||
|
@@ -264,6 +265,10 @@ class CategoryRenderer { | |
const clumpTmpl = this.dom.cloneTemplate('#tmpl-lh-clump', this.templateContext); | ||
const clumpElement = this.dom.find('.lh-clump', clumpTmpl); | ||
|
||
if (clumpId === 'warning') { | ||
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. do we want it to be an open 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. renderUnexpandableClump's don't have titles/descriptions so .... let's stick with this approach. |
||
clumpElement.setAttribute('open', ''); | ||
} | ||
|
||
const summaryInnerEl = this.dom.find('.lh-audit-group__summary', clumpElement); | ||
const chevronEl = summaryInnerEl.appendChild(this._createChevron()); | ||
chevronEl.title = Util.UIStrings.auditGroupExpandTooltip; | ||
|
@@ -333,6 +338,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 | ||
|
@@ -344,7 +357,9 @@ class CategoryRenderer { | |
return scoreDisplayMode; | ||
} | ||
|
||
if (Util.showAsPassed(auditRef.result)) { | ||
if (this._auditHasWarning(auditRef)) { | ||
return 'warning'; | ||
} else if (Util.showAsPassed(auditRef.result)) { | ||
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. hmm, we should figure out what we want the priority order to be here. Does 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. changed to require a passing state for all warnings. |
||
return 'passed'; | ||
} else { | ||
return 'failed'; | ||
|
@@ -382,6 +397,7 @@ class CategoryRenderer { | |
/** @type {Map<TopLevelClumpId, Array<LH.ReportResult.AuditRef>>} */ | ||
const clumps = new Map(); | ||
clumps.set('failed', []); | ||
clumps.set('warning', []); | ||
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. should add 'warning' to the list of statuses in the method jsdoc for |
||
clumps.set('manual', []); | ||
clumps.set('passed', []); | ||
clumps.set('notApplicable', []); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. */ | ||
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. Need some context for "warnings" for the translators ("contain warnings" is pretty ambiguous in isolation)...what they are, how they're used sort of thing. See below for examples. |
||
warningAuditsGroupTitle: 'Audits with warnings', | ||
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. I feel like we should be more specific with something like 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. wb |
||
/** 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. */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,6 +129,17 @@ describe('CategoryRenderer', () => { | |
assert.ok(warningEl.textContent.includes(auditResult.warnings[1]), '2nd warning provided'); | ||
}); | ||
|
||
it('expands warning 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. nit: move this under the |
||
const category = sampleResults.reportCategories.find(c => c.id === 'pwa'); | ||
const categoryClone = JSON.parse(JSON.stringify(category)); | ||
categoryClone.auditRefs[0].result.warnings = ['Some warning']; | ||
|
||
const auditDOM = renderer.render(categoryClone, sampleResults.categoryGroups); | ||
const warningClumpEl = auditDOM.querySelector('.lh-clump--warning'); | ||
const isExpanded = warningClumpEl.hasAttribute('open'); | ||
assert.ok(isExpanded, 'Warning audit group should be expanded by default'); | ||
}); | ||
|
||
it('renders a category', () => { | ||
const category = sampleResults.reportCategories.find(c => c.id === 'pwa'); | ||
const categoryDOM = renderer.render(category, sampleResults.categoryGroups); | ||
|
@@ -314,16 +325,18 @@ describe('CategoryRenderer', () => { | |
}); | ||
}); | ||
|
||
describe('clumping passed/failed/manual', () => { | ||
describe('clumping passed/failed/warning/manual', () => { | ||
brendankenny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 failedAudits = elem.querySelectorAll('.lh-clump--failed .lh-audit'); | ||
const warningAudits = elem.querySelectorAll('.lh-clump--warning .lh-audit'); | ||
const manualAudits = elem.querySelectorAll('.lh-clump--manual .lh-audit'); | ||
|
||
assert.equal(passedAudits.length, 4); | ||
assert.equal(failedAudits.length, 8); | ||
assert.equal(warningAudits.length, 0); | ||
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 test doesn't end up testing much :) WDYT about adding a warning to one of the passed audits? 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. There are no passing audits with warnings in the sample json, and I'm not sure how to update the source appropriately. fine to just manually set some warnings in the test case? 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.
perfect |
||
assert.equal(manualAudits.length, 3); | ||
}); | ||
|
||
|
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.
it's unfortunate we used "group" for these :/