Skip to content

Use get_the_post_thumbnail function in the cover block. #40853

Merged
gziolo merged 3 commits intotrunkfrom
fix/function-usage
May 6, 2022
Merged

Use get_the_post_thumbnail function in the cover block. #40853
gziolo merged 3 commits intotrunkfrom
fix/function-usage

Conversation

@spacedmonkey
Copy link
Copy Markdown
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 requested a review from ajitbohra as a code owner May 5, 2022 15:14
@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
@spacedmonkey spacedmonkey requested a review from scruffian May 5, 2022 15:44
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

"data-object-position" => $object_position,
);

$image = get_the_post_thumbnail( null, 'post-thumbnail', $attr );
Copy link
Copy Markdown
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
Copy Markdown
Member

Excellent, thanks for working on this @spacedmonkey!

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

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
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.

@spacedmonkey spacedmonkey added [Type] Bug An existing feature does not function as intended labels May 6, 2022
@aristath
Copy link
Copy Markdown
Member

aristath commented May 6, 2022

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

Copy link
Copy Markdown
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
Copy Markdown
Member

gziolo commented May 6, 2022

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

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 <aristath@gmail.com>
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.

7 participants