Skip to content
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

Use get_the_post_thumbnail function in the cover block. #40853

Merged
merged 3 commits into from
May 6, 2022

Conversation

spacedmonkey
Copy link
Member

What?

Follow on from #39658.

Why?

Improved logic, to that if cover is not an image, then do not both to load feature image.
Ensure that get_the_post_thumbnail function is called, so that filters are run correctly.
Ensure that update_post_thumbnail_cache is called, so that feature image are loaded into memory.

How?

Testing Instructions

Screenshots or screencast

@spacedmonkey spacedmonkey added [Type] Performance Related to performance efforts [Block] Cover Affects the Cover Block - used to display content laid over a background image labels May 5, 2022
@spacedmonkey spacedmonkey self-assigned this May 5, 2022
@spacedmonkey spacedmonkey changed the title Use get_the_post_thumbnail function is used for cover block. Use get_the_post_thumbnail function in the cover block. May 5, 2022
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacedmonkey Great catch! This does not only come with improvements for consistency and performance, but also accessibility. 🎉

Please check the CI failures. Other than that I'm happy to approve it.


$image = sprintf(
$image_template,
esc_attr( get_the_post_thumbnail_caption() ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was in fact incorrect and could result in accessibility problems, since it was using the caption as the alt attribute, when in fact WordPress images have their own dedicated field to define the alternative text.

So fixing that is another benefit of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch, I think it resulted like this as I didn't consider that featured images are their own kind of entity in WP, instead treated it as another simple image added to the cover block.

);

$image = get_the_post_thumbnail( null, 'post-thumbnail', $attr );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes total sense, we shouldn't manually create img tags in WordPress core when dealing with attachments.

@adamsilverstein
Copy link
Member

Excellent, thanks for working on this @spacedmonkey!

Comment on lines +42 to +44
if ( in_the_loop() ) {
update_post_thumbnail_cache();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

The change adds several attributes to the image tag

  • loading
  • srcset
  • sizes
  • width
  • height
  • class changes from wp-block-cover__image-background to wp-block-cover__image-background wp-post-image

I think all these are good things provided the width and height don't affect object fit.

No changes when the image is used as a background URL.

@gziolo gziolo added the Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label May 6, 2022
@spacedmonkey spacedmonkey added [Type] Bug An existing feature does not function as intended labels May 6, 2022
@aristath
Copy link
Member

aristath commented May 6, 2022

I fixed a couple of PHP Coding-Standards issues, once test pass this can be merged 👍

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to leave the 3rd approval here :D

@gziolo gziolo merged commit e1bb280 into trunk May 6, 2022
@gziolo gziolo deleted the fix/function-usage branch May 6, 2022 13:35
@github-actions github-actions bot added this to the Gutenberg 13.3 milestone May 6, 2022
@gziolo
Copy link
Member

gziolo commented May 6, 2022

I cherry-picked this PR for WordPress 6.0 RC2 with 93befd1.

@gziolo gziolo removed the Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label May 6, 2022
gziolo pushed a commit that referenced this pull request May 6, 2022
* Improve logic.

* Improve logic.

* Coding Standards: Using single-quotes if double-quotes are not required

Co-authored-by: Ari Stathopoulos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Bug An existing feature does not function as intended [Type] Performance Related to performance efforts
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

7 participants