Fix video CLS netting +10 lighthouse points #2750
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## stage #2750 +/- ##
==========================================
- Coverage 95.90% 95.88% -0.02%
==========================================
Files 173 173
Lines 46051 46027 -24
==========================================
- Hits 44163 44131 -32
- Misses 1888 1896 +8 ☔ View full report in Codecov by Sentry. |
|
Seems like PSI is having a bad day. Can be ignored. |
mokimo
requested review from
narcis-radu,
overmyheadandbody,
SilviuLCF,
Dli3 and
robert-bogos
There was a problem hiding this comment.
I noticed something interesting on the Bacom page that might deserve another look, although as part of another story, since this is the current behavior on production today as well.
I captured a couple of frames and noticed that:
- initially the text is on the right side, the left side is empty;
- the text then gets pushed to the left (CLS?);
- background gets applied;
- CTAs slightly shrink, leading to a bit more content jump (CLS?).
Here's a stop motion animation of what's going on on the left side, that's what I wanted to focus on:
Bacom.Video.mov
|
@overmyheadandbody did you encounter this on the Any special page you still encounter this on? |
|
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
|
@SilviuLCF can you see if you can reproduce what @overmyheadandbody saw? 🤔 |
|
@mokimo - only on Bacom do I see this behavior, both on the before and the after links. All other are very "stable". It's likely there's some sort of override there that should be analyzed as part of a different story, since I suspect it has nothing to do with the video. CC: @SilviuLCF |
There was a problem hiding this comment.
@mokimo @overmyheadandbody
Cannot reproduce it anymore on after url on bacom.
I will still keep an eye on it.
PR is verified and can be merged to STAGE
|
Skipped merging 2750: Fix video CLS netting +10 lighthouse points due to failing checks |
2 similar comments
|
Skipped merging 2750: Fix video CLS netting +10 lighthouse points due to failing checks |
|
Skipped merging 2750: Fix video CLS netting +10 lighthouse points due to failing checks |
|
Skipped merging 2750: Fix video CLS netting +10 lighthouse points due to failing checks |
4 similar comments
|
Skipped merging 2750: Fix video CLS netting +10 lighthouse points due to failing checks |
|
Skipped merging 2750: Fix video CLS netting +10 lighthouse points due to failing checks |
|
Skipped merging 2750: Fix video CLS netting +10 lighthouse points due to failing checks |
|
Skipped merging 2750: Fix video CLS netting +10 lighthouse points due to failing checks |
|
Skipped merging 2750: Fix video CLS netting +10 lighthouse points due to failing checks |
* fix cls by adding the videoEl without a source * adapt the video hover and in view port play * Remove no-lazy and return earlier * Fix linting issue * Consolidate duplicated logic * Move functions into the init function * Move root margin and use optional chaining * Only query for the videoEl once
|
Moving some of the tests to the description: Onviewport play
Only desktop (mobile should not load the video for perf reasons)
CLS
Bacom tests |
|
Skipped merging 2750: Fix video CLS netting +10 lighthouse points due to failing checks |
Original PR: #2724
Revert PR: #2748
Resolves: MWPW-156749
Test URLs:
Onhover play