-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Conversation
@@ -338,7 +372,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 comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes a bug where the title
of the "All" filter button is "undefined"
@@ -197,6 +197,8 @@ declare module Result { | |||
|
|||
/** Gather mode used to collect artifacts. */ | |||
type GatherMode = 'navigation'|'timespan'|'snapshot'; | |||
|
|||
type MetricAcronym = 'FCP' | 'LCP' | 'TBT' | 'CLS' | 'INP' | 'SI' | 'TTI' | 'FMP'; |
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.
ping @connorjclark |
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.
lgtm!
*/ | ||
renderAudit(audit) { | ||
const component = this.dom.createComponent('audit'); | ||
return this.populateAuditValues(audit, component); | ||
return /** @type {HTMLElement} */ (this.populateAuditValues(audit, component)); |
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.
hidden
is not a property on Element
but it is on HTMLElement
#15445 (comment)
Metric filter was still using
relevantAudits
which is just a manually curated list that we haven't touched in a while. This PR usesmetricSavings
instead to determine filter applicability and sorts the audits on the filtered metric.