-
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(performance): use metric savings for metric filter #15540
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
7943b24
start
adamraine 12814f9
types
adamraine b104ab5
rm comment
adamraine 165f4bd
fix unit
adamraine 9863e3f
unit test
adamraine 2cef5e1
Merge branch 'main' into new-perf-audit-filter
adamraine 6a75288
format
adamraine 2b6d1f5
nit
adamraine 1acd844
Merge branch 'main' into new-perf-audit-filter
adamraine ed2f19c
nits
adamraine File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
*/ | ||
|
||
/** @typedef {import('./dom.js').DOM} DOM */ | ||
/** @typedef {LH.Result.MetricAcronym | 'All'} FilterType */ | ||
|
||
import {CategoryRenderer} from './category-renderer.js'; | ||
import {ReportUtils} from './report-utils.js'; | ||
|
@@ -211,73 +212,106 @@ export class PerformanceCategoryRenderer extends CategoryRenderer { | |
filmstripEl && timelineEl.append(filmstripEl); | ||
} | ||
|
||
const filterableMetrics = metricAudits.filter(a => !!a.relevantAudits); | ||
// TODO: only add if there are opportunities & diagnostics rendered. | ||
if (filterableMetrics.length) { | ||
this.renderMetricAuditFilter(filterableMetrics, element); | ||
} | ||
const allInsights = category.auditRefs | ||
.filter(audit => this._isPerformanceInsight(audit)) | ||
.map(auditRef => { | ||
const {overallImpact, overallLinearImpact} = this.overallImpact(auditRef, metricAudits); | ||
const guidanceLevel = auditRef.result.guidanceLevel || 1; | ||
const auditEl = this.renderAudit(auditRef); | ||
|
||
return {auditRef, auditEl, overallImpact, overallLinearImpact, guidanceLevel}; | ||
}); | ||
|
||
// Diagnostics | ||
const diagnosticAudits = category.auditRefs | ||
.filter(audit => this._isPerformanceInsight(audit)) | ||
.filter(audit => !ReportUtils.showAsPassed(audit.result)); | ||
|
||
/** @type {Array<{auditRef:LH.ReportResult.AuditRef, overallImpact: number, overallLinearImpact: number, guidanceLevel: number}>} */ | ||
const auditImpacts = []; | ||
diagnosticAudits.forEach(audit => { | ||
const { | ||
overallImpact: overallImpact, | ||
overallLinearImpact: overallLinearImpact, | ||
} = this.overallImpact(audit, metricAudits); | ||
const guidanceLevel = audit.result.guidanceLevel ?? 1; | ||
auditImpacts.push({auditRef: audit, overallImpact, overallLinearImpact, guidanceLevel}); | ||
}); | ||
const diagnosticAudits = allInsights | ||
.filter(audit => !ReportUtils.showAsPassed(audit.auditRef.result)); | ||
|
||
const passedAudits = allInsights | ||
.filter(audit => ReportUtils.showAsPassed(audit.auditRef.result)); | ||
|
||
const [groupEl, footerEl] = this.renderAuditGroup(groups['diagnostics']); | ||
groupEl.classList.add('lh-audit-group--diagnostics'); | ||
|
||
/** | ||
* @param {FilterType} acronym | ||
*/ | ||
function refreshFilteredAudits(acronym) { | ||
for (const audit of allInsights) { | ||
if (acronym === 'All') { | ||
audit.auditEl.hidden = false; | ||
} else { | ||
const shouldHide = audit.auditRef.result.metricSavings?.[acronym] === undefined; | ||
audit.auditEl.hidden = shouldHide; | ||
} | ||
} | ||
|
||
diagnosticAudits.sort((a, b) => { | ||
// If the score display mode is "metricSavings", the `score` will be a coarse indicator of the overall impact. | ||
// Therefore, it makes sense to sort audits by score first to ensure visual clarity with the score icons. | ||
const scoreA = a.auditRef.result.scoreDisplayMode === 'informative' | ||
? 100 | ||
: Number(a.auditRef.result.score); | ||
const scoreB = b.auditRef.result.scoreDisplayMode === 'informative' | ||
? 100 | ||
: Number(b.auditRef.result.score); | ||
if (scoreA !== scoreB) return scoreA - scoreB; | ||
|
||
// If there is a metric filter applied, we should sort by the impact to that specific metric. | ||
if (acronym !== 'All') { | ||
const aSavings = a.auditRef.result.metricSavings?.[acronym] ?? -1; | ||
const bSavings = b.auditRef.result.metricSavings?.[acronym] ?? -1; | ||
if (aSavings !== bSavings) return bSavings - aSavings; | ||
} | ||
|
||
// Overall impact is the estimated improvement to the performance score | ||
if (a.overallImpact !== b.overallImpact) { | ||
return b.overallImpact * b.guidanceLevel - a.overallImpact * a.guidanceLevel; | ||
} | ||
|
||
// Fall back to the linear impact if the normal impact is rounded down to 0 | ||
if ( | ||
a.overallImpact === 0 && b.overallImpact === 0 && | ||
a.overallLinearImpact !== b.overallLinearImpact | ||
) { | ||
return b.overallLinearImpact * b.guidanceLevel - a.overallLinearImpact * a.guidanceLevel; | ||
} | ||
|
||
// Audits that have no estimated savings should be prioritized by the guidance level | ||
return b.guidanceLevel - a.guidanceLevel; | ||
}); | ||
|
||
auditImpacts.sort((a, b) => { | ||
// Performance diagnostics should only have score display modes of "informative" and "metricSavings" | ||
// If the score display mode is "metricSavings", the `score` will be a coarse approximation of the overall impact. | ||
// Therefore, it makes sense to sort audits by score first to ensure visual clarity with the score icons. | ||
const scoreA = a.auditRef.result.scoreDisplayMode === 'informative' | ||
? 100 | ||
: Number(a.auditRef.result.score); | ||
const scoreB = b.auditRef.result.scoreDisplayMode === 'informative' | ||
? 100 | ||
: Number(b.auditRef.result.score); | ||
if (scoreA !== scoreB) return scoreA - scoreB; | ||
|
||
// Overall impact is the estimated improvement to the performance score | ||
if (a.overallImpact !== b.overallImpact) { | ||
return b.overallImpact * b.guidanceLevel - a.overallImpact * a.guidanceLevel; | ||
for (const audit of diagnosticAudits) { | ||
groupEl.insertBefore(audit.auditEl, footerEl); | ||
} | ||
} | ||
|
||
// Fall back to the linear impact if the normal impact is rounded down to 0 | ||
if ( | ||
a.overallImpact === 0 && b.overallImpact === 0 && | ||
a.overallLinearImpact !== b.overallLinearImpact | ||
) { | ||
return b.overallLinearImpact * b.guidanceLevel - a.overallLinearImpact * a.guidanceLevel; | ||
/** @type {Set<string>} */ | ||
const filterableMetricAcronyms = new Set(); | ||
for (const audit of diagnosticAudits) { | ||
const metricSavings = audit.auditRef.result.metricSavings || {}; | ||
for (const [key, value] of Object.entries(metricSavings)) { | ||
if (typeof value === 'number') filterableMetricAcronyms.add(key); | ||
} | ||
} | ||
|
||
// Audits that have no estimated savings should be prioritized by the guidance level | ||
return b.guidanceLevel - a.guidanceLevel; | ||
}); | ||
const filterableMetrics = | ||
metricAudits.filter(a => a.acronym && filterableMetricAcronyms.has(a.acronym)); | ||
|
||
if (auditImpacts.length) { | ||
const [groupEl, footerEl] = this.renderAuditGroup(groups['diagnostics']); | ||
auditImpacts.forEach(item => groupEl.insertBefore(this.renderAudit(item.auditRef), footerEl)); | ||
groupEl.classList.add('lh-audit-group--diagnostics'); | ||
element.append(groupEl); | ||
// TODO: only add if there are opportunities & diagnostics rendered. | ||
if (filterableMetrics.length) { | ||
this.renderMetricAuditFilter(filterableMetrics, element, refreshFilteredAudits); | ||
} | ||
|
||
// Passed audits | ||
const passedAudits = category.auditRefs | ||
.filter(audit => | ||
this._isPerformanceInsight(audit) && ReportUtils.showAsPassed(audit.result)); | ||
refreshFilteredAudits('All'); | ||
|
||
if (diagnosticAudits.length) { | ||
element.append(groupEl); | ||
} | ||
|
||
if (!passedAudits.length) return element; | ||
|
||
const clumpOpts = { | ||
auditRefs: passedAudits, | ||
auditRefsOrEls: passedAudits.map(audit => audit.auditEl), | ||
groupDefinitions: groups, | ||
}; | ||
const passedElem = this.renderClump('passed', clumpOpts); | ||
|
@@ -318,16 +352,17 @@ export class PerformanceCategoryRenderer extends CategoryRenderer { | |
* Render the control to filter the audits by metric. The filtering is done at runtime by CSS only | ||
* @param {LH.ReportResult.AuditRef[]} filterableMetrics | ||
* @param {HTMLDivElement} categoryEl | ||
* @param {(acronym: FilterType) => void} onFilterChange | ||
*/ | ||
renderMetricAuditFilter(filterableMetrics, categoryEl) { | ||
renderMetricAuditFilter(filterableMetrics, categoryEl, onFilterChange) { | ||
const metricFilterEl = this.dom.createElement('div', 'lh-metricfilter'); | ||
const textEl = this.dom.createChildOf(metricFilterEl, 'span', 'lh-metricfilter__text'); | ||
textEl.textContent = Globals.strings.showRelevantAudits; | ||
|
||
const filterChoices = /** @type {LH.ReportResult.AuditRef[]} */ ([ | ||
({acronym: 'All'}), | ||
const filterChoices = [ | ||
/** @type {const} */ ({acronym: 'All'}), | ||
...filterableMetrics, | ||
]); | ||
]; | ||
|
||
// Form labels need to reference unique IDs, but multiple reports rendered in the same DOM (eg PSI) | ||
// would mean ID conflict. To address this, we 'scope' these radio inputs with a unique suffix. | ||
|
@@ -341,7 +376,7 @@ export class PerformanceCategoryRenderer extends CategoryRenderer { | |
|
||
const labelEl = this.dom.createChildOf(metricFilterEl, 'label', 'lh-metricfilter__label'); | ||
labelEl.htmlFor = elemId; | ||
labelEl.title = metric.result?.title; | ||
labelEl.title = 'result' in metric ? metric.result.title : ''; | ||
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 fixes a bug where the |
||
labelEl.textContent = metric.acronym || metric.id; | ||
|
||
if (metric.acronym === 'All') { | ||
|
@@ -357,17 +392,7 @@ export class PerformanceCategoryRenderer extends CategoryRenderer { | |
} | ||
categoryEl.classList.toggle('lh-category--filtered', metric.acronym !== 'All'); | ||
|
||
for (const perfAuditEl of categoryEl.querySelectorAll('div.lh-audit')) { | ||
if (metric.acronym === 'All') { | ||
perfAuditEl.hidden = false; | ||
continue; | ||
} | ||
|
||
perfAuditEl.hidden = true; | ||
if (metric.relevantAudits && metric.relevantAudits.includes(perfAuditEl.id)) { | ||
perfAuditEl.hidden = false; | ||
} | ||
} | ||
onFilterChange(metric.acronym || 'All'); | ||
|
||
// Hide groups/clumps if all child audits are also hidden. | ||
const groupEls = categoryEl.querySelectorAll('div.lh-audit-group, details.lh-audit-group'); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
which part required this change to return type?
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.
https://github.com/GoogleChrome/lighthouse/blob/new-perf-audit-filter/report/renderer/performance-category-renderer.js#L241
hidden
is not a property onElement
but it is onHTMLElement