-
Notifications
You must be signed in to change notification settings - Fork 29
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
Stop lazy loading of content images above the fold to improve LCP #72
Conversation
*/ | ||
function skip_lazyloading_on_homepage_images( $omit_threshold ) { | ||
if ( is_home() ) { | ||
return 2; |
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.
Draft value
9e19cad
to
6a22ec2
Compare
6a22ec2
to
566b6e4
Compare
566b6e4
to
f7d0102
Compare
f7d0102
to
e7a6e65
Compare
The first issue I ran into is that the lazy attribute is only added to images on the current site, so my local site wasn't adding it because it was pulled from I tracked how that threshold value is used, and it's only used if the image is in the content, otherwise it defaults to // Only elements with 'the_content' or 'the_post_thumbnail' context have special handling.
if ( 'the_content' !== $context && 'the_post_thumbnail' !== $context ) {
return 'lazy';
} So instead, I tried filtering the attribute itself when it's added, with The value of
Anyway, I think e7a6e65 is a workaround we can launch with, and we can think about a future-safe solution later. |
Nice one Kelly! That's exactly where I was trying to get to, I hadn't found that particular filter though. This is a good workaround for now, @adamwoodnz let's merge it if it works. As a more flexible solution going forward, instead of string-matching the file name, could we use something like a special classname or other attribute on the image to trigger it? That way it could be more explicitly controlled in the template rather than hidden away in a magic filter. Definitely not a blocker for merging though. |
👍 testing in progress |
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.
LGTM, couldn't get a local image working, but tested in Sandbox
I think we could apply a |
Partially fixes #45
Content images which are above the fold shouldn't be lazy loaded as it affects the Largest Contentful Paint of the page, a key performance metric.
This PR adds a filter to skip X number of images as outlined on https://make.wordpress.org/core/2021/12/29/enhanced-lazy-loading-performance-in-5-9/, however the filter doesn't fire, @tellyworth suggested perhaps related to the content being in a pattern. Not sure if this being page content rather than post content makes a difference either.
The image in question is actually the first content image in the page, so if I understand correctly it should not be lazy loaded by default. It seems we're doing something differently to the way this optimization expects.
Screenshots
How to test the changes in this Pull Request: