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.
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
new_audit: layout-shifts with estimated root causes #15703
Changes from 39 commits
b9e5501
c030cf2
e0ab6a1
de3af7f
a5c3a8f
bfd0edb
7c1e444
ad7ea6a
4e63154
49c35fb
8ed3283
315f956
0c05330
f175483
1fe9c4e
b7165ec
4d30fff
e8b5b34
84c2382
0a98d92
4a2b384
cb9e425
243fb31
44562d4
70fb9b6
e7fe8bd
002b1ea
f30243d
f1accfd
cfa9e05
23997ca
e0639f8
9a35241
b77fbdd
2c8a991
b166a93
b74d3a8
93cb90b
a7512c4
3c93946
271759d
7cc0bdb
749fb22
d84048c
338b8e1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
layout-shifts
audit that takes the elements from the old auditThere 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.
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.
andres commented that "I remember this one getting noisy at some point on apps with many ads". Something for us to keep in mind. Based on the impl, I don't think 0x0 iframes will be counted, but 1x1 are.
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.
Is there a behavioral difference between 1x1 and 0x0 iframes, or is that purely a developer mistake resulting in (the most minor) shift?