-
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
new_audit: layout-shifts with estimated root causes #15703
Conversation
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 file will be deleted whenever we properly move the shared trace engine to a separate repository, with properly emitted types. Or maybe we can get that while it still lives in CDT. I haven't investigated the problem yet, but it was fast enough to just copy/paste 95% of the types and hand write the little we really need so far.
1461e63
to
67845f5
Compare
const TraceHandlers = TraceEngine.Handlers.ModelHandlers; | ||
/** @type {import('../../types/trace-engine.js').RootCauses & typeof import('../../types/trace-engine.js').RootCauses} */ | ||
const RootCauses = TraceEngine.RootCauses.RootCauses.RootCauses; |
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.
@paulirish this is what i did for the types for now
Quite lost on how this could have changed |
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.
Can you add a image and/or example report?
|
||
const UIStrings = { | ||
/** Descriptive title of a diagnostic audit that provides the top elements affected by layout shifts. */ | ||
title: 'Avoid large layout shifts', |
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 the same title as layout-shift-elements
. I understand why since the call to action is the same, but this makes me wonder if we should somehow combine the audits (e.g. multiple tables in the same 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.
In the linked doc, we left the fate of this other audit unresolved: We should make it hidden or remove it or make it informative…
.
wdyt of marking it informative?
can also change title to Avoid large layout shifts (impacted elements)
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 think I'm leaning towards the hide/remove option at this point. This new audit seems generally better than the old one and less of an information dump. That being said, if we still see value in the element summary my preferences go as follows:
- Hide/remove the old audit
- Add a second table to the new
layout-shifts
audit that takes the elements from the old audit - Make the old audit informative
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.
+1 to ditching the old one if possible (hidden for now to not break anyone makes sense). Is it possible to make sure all the information in the old audit is in the new audit (either improved or at least as-is)? e.g. for shifts without a known root cause
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.
OK, let's hide it.
Is it possible to make sure all the information in the old audit is in the new audit (either improved or at least as-is)?
This audit won't hide a layout shift if it has no determined root cause. root cause is just an enhancement.
For the top 15 layout shifts, we'll show the the largest impacted element that moves in each shift, whereas the previous audit would show the top impacted elements despite being from the same shift or not. So it isn't 1:1, but we'll be showing all the biggest impacted nodes still. Should be higher useful info : noise ratio
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.
Yeah, sorry, I commented without reading through the code or trying it out :) Sounds great to me!
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.
Would also like to resolve #15703 (comment)
The failing test is flaky and unrelated: #15729 |
Builds off of #15702
Introduces
layout-shifts
, an audit similar tolayout-shift-elements
but focused on shifts instead of individually impacted nodes. Additionally, makes use of the CLS root causes from the shared trace engine, and creates sub items if any possible root cause is found for a given shift.Doc.
outdated example report