-
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
core(largest-contentful-paint-element): add LCP savings #15178
Conversation
@@ -152,6 +158,9 @@ class LargestContentfulPaintElement extends Audit { | |||
score: 1, | |||
displayValue, | |||
details, | |||
metricSavings: { | |||
LCP: metricLcp || 0, |
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 doesn't seem very meaningful. If the goal is just to be at the top of whatever sorting we have later, add that as a comment. cuz this isn't really representative of a savings opportunity.
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 is why we need to balance the impact ranking with the guidance ranking when we sort audits. This audit will have a consistently high impact but we also gave it a very low guidance ranking: https://github.com/GoogleChrome/lighthouse/pull/15025/files#diff-de507a9267e42ea608e9eab1c02d92e8283d073b43edf8b30fdd9fb2084115ea. The audit doesn't give any guidance, but it is still an extremely useful tool to improve LCP and that should be reflected in the savings. It's a similar situation to #15070
Currently this audit sits near the bottom of every report. In my personal opinion, elevating this audit in the sort order would be a good thing.
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.
I understand how this is a knob to turn to make this audit higher in the future sorting, it just doesn't mesh that this property is called metricSavings
and conceptually this audit isn't something that would result in savings, or even anything to fix. That's why I suggested at least a comment. It's also a hint that maybe the property metricSavings
is not the best name, but I don't have a serious suggestion for an alternative.
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.
Gotcha, I'll add a comment like you said. Also, metricImpact
is an alternative name that might make more sense conceptually.
conceptually this audit isn't something that would result in savings, or even anything to fix.
FWIW my thinking is that the LCP element is the thing to fix/improve for this audit.
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 the "thing" but the savings given is not realistic, because a LCP of zero is not realistic.
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 is true. WDYT about using lcp - fcp
as the savings?
Edit: Could also do Math.max(lcp - 1000, 0)
since 1000ms is what our score calculator uses for 100% on LCP. I have seen LCPs lower than this though.
return { | ||
score: null, | ||
notApplicable: true, | ||
metricSavings: {LCP: 0}, |
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.
I've generally been of the mindset that we should avoid adding any of the usual stuff to audits in the notApplicable
case. It's not a hard and fast rule, but when looking at an LHR, you generally shouldn't have to wonder if a value on an audit is important. The whole thing has been marked not applicable; it's not that there's no savings to be had, it's that this audit is irrelevant for this particular page load.
(the only exception might be debugdata
if we ever want to pipe through information on why something was marked n/a to http archive or wherever, but I don't know if that's ever come up before)
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.
True, but I was planning for metricSavings
to eventually replace our current metric applicability lists. Removing metricSavings
when an audit is N/A means that it will always be hidden when the user filters to any metric.
Not necessarily a bad UX, but it's also not how we do the filters right now. Knowing that an audit can affect a certain metric even when it's N/A can be useful information IMO.
No description provided.