-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Ensure post-featured-image block is in_the_loop() for BC with core and plugins, and to fix lazy-loading #45534
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
@Mamaduka I'd like to get this on your radar. Can we prioritize this PR for an upcoming Gutenberg release and then a merge to WP core |
Thanks for the ping, @felixarntz! This is on my to-do list, and I will try to test it as soon as possible thoroughly. |
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.
The changes test well for me, and considering we already have similar code in the Post Content block for BC, I think this is safe to land.
Tests
- I confirmed that thumbnail previews work as before.
- The first featured image doesn't have the
loading="lazy"
attribute when testing with Ensure that first content image is not lazy-loaded in block themes either wordpress-develop#3560.
A follow-up
We'll need a similar follow-up for the Cover block when it renders the featured image, but this doesn't have to be a blocker for WordPress/wordpress-develop#3560.
if ( ! in_the_loop() && have_posts() ) { | ||
the_post(); | ||
} | ||
|
||
$is_link = isset( $attributes['isLink'] ) && $attributes['isLink']; | ||
$size_slug = isset( $attributes['sizeSlug'] ) ? $attributes['sizeSlug'] : 'post-thumbnail'; | ||
$post_title = trim( strip_tags( get_the_title( $post_ID ) ) ); |
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.
Should we remove the $post_ID
usage from the context
now that we set the global post object? I think we did something similar in #37622.
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.
@Mamaduka Oh you mean not pass $post_ID
to these functions and instead rely on the default which would be the same current post anyway? That seems reasonable to me.
We could do this here and also further down on get_the_post_thumbnail()
and get_the_permalink()
.
Do you think I should add it to this PR or rather in a separate PR?
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.
Oh you mean not pass $post_ID to these functions and instead rely on the default which would be the same current post anyway? That seems reasonable to me.
We could do this here and also further down on get_the_post_thumbnail() and get_the_permalink().
Correct.
Do you think I should add it to this PR or rather in a separate PR?
We can do it in a separate PR if you prefer.
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.
@Mamaduka Sounds good, I'll open a follow up PR once this is merged. Is this waiting for anything else or should we merge it?
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.
Feel free to merge 👍
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, that's the main reason I'm asking. Is there any way we can cherry-pick / backport this change to core sooner? Happy to help as much as I can.
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.
We'll have to do a minor release for the packages outside the release scope, but I don't know what's the policy here. I'm going to ping folks who have done more release than me.
Cc. @noisysocks, @youknowriad
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.
We'll have to do a minor release for the packages outside the release scope, but I don't know what's the policy here. I'm going to ping folks who have done more release than me.
This is something we do often I think. We'll have to cherry-pick the commits to the right wp/*
branch and perform a npm patch release from there. I'm not entirely certain with the process (as it changed lately a bit) but I know that it's documented.
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.
Thanks, Riad! I'm familiar with the new process and can do a patch release.
@felixarntz, I can do a package release and prepare core PR early next week.
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.
Perfect, thanks both! Let me know if I can help with anything like reviews or so.
Fixing the issue that featured images are always lazy-loaded in block themes required an upstream Gutenberg change WordPress/gutenberg#45534. This changeset updates packages in order to unblock the remainder of the fix in `trunk`. Props mamaduka. See #56930. git-svn-id: https://develop.svn.wordpress.org/trunk@55079 602fd350-edb4-49c9-b593-d223f7449a82
Fixing the issue that featured images are always lazy-loaded in block themes required an upstream Gutenberg change WordPress/gutenberg#45534. This changeset updates packages in order to unblock the remainder of the fix in `trunk`. Props mamaduka. See #56930. Built from https://develop.svn.wordpress.org/trunk@55079 git-svn-id: http://core.svn.wordpress.org/trunk@54612 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Fixing the issue that featured images are always lazy-loaded in block themes required an upstream Gutenberg change WordPress/gutenberg#45534. This changeset updates packages in order to unblock the remainder of the fix in `trunk`. Props mamaduka. See #56930. Built from https://develop.svn.wordpress.org/trunk@55079 git-svn-id: https://core.svn.wordpress.org/trunk@54612 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Fixing the issue that featured images are always lazy-loaded in block themes required an upstream Gutenberg change WordPress/gutenberg#45534. This changeset updates packages in order to unblock the remainder of the fix in `trunk`. Props mamaduka. See #56930. Built from https://develop.svn.wordpress.org/trunk@55079 git-svn-id: http://core.svn.wordpress.org/trunk@54612 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@felixarntz Just a heads up that this PR seems to have broken the core loop if the Post Featured Image block is hooked before it: https://core.trac.wordpress.org/ticket/58027 |
@tomusborne Thank you, looking into it! |
What?
This PR is part of the solution from WordPress/wordpress-develop#3560 (see that PR for additional context), which addresses https://core.trac.wordpress.org/ticket/56930. The problem here specifically also addresses #41783.
Why?
Currently the logic to not lazy-load the first featured image or in-content image is not working for block themes. WordPress/wordpress-develop#3560 is the full solution to that problem, however this PR here is necessary since the block itself comes from Gutenberg. We should therefore merge and port this to WP core's blocks soon (in time for WP 6.2).
How?
WP core's logic to determine whether to lazy-load an image expects to run
in_the_loop()
when the main content is rendered. Since block themes by default may not enter the loop initially, this extra call here needs to be added, as currently the lack of it is a backward compatibility break causing the problem outlined above. This fix is already present in thepost-content
block, so it's simply brought over here as well.Testing Instructions
trunk
, the first featured image will haveloading="lazy"
on it, which is incorrect. With Ensure that first content image is not lazy-loaded in block themes either wordpress-develop#3560, it will not have the attribute as expected.