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.
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.
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.
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.
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.
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.
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.
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.
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.