Media Text Block: Image size option for featured image #67464
Conversation
akasunil
added
[Type] Enhancement
|
Size Change: +163 B (+0.01%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
…nt/resolution-option-for-featured-image-media-text-block
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in 3f9ad85. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12194092232
|
There was a problem hiding this comment.
Thanks for putting this together @akasunil 👍
It looks like this PR makes an unintended change to the way image urls fallback i.e. they no longer prefer the large size rather than full if mediaSizeSlug isn't set.
Can you also update the PR test instructions to cover this aspect of the PR's changes?
…nt/resolution-option-for-featured-image-media-text-block
There was a problem hiding this comment.
I should have been clearer with my last review comment, sorry 🙏
The unexpected change of behaviour was the real problem not the comment. The original behaviour should persist unless there was a good reason to change it.
In making changes to existing code or styles it's really helpful to keep Chesterton's Fence in mind. The TL;DR on that principle is we need to understand why something is in place before we change or move it to avoid unintended side effects.
Please checkout the attached video to see the problem that exists in the trunk. Edit-Post-.-gutenberg-.-WordPress.webmIf resolution size is set to |
It might help if you can articulate the steps to reproduce an issue and what reviewers should be looking for. The same should apply to my last review too even though I was short on time.
The unexpected change in behaviour wasn't the desire to use any selected resolution but the change in behaviour when a resolution hasn't been selected. With no resolution selected, instead of preferring the large size, falling back to full, the code in question now returns
This test instruction clearly states a change in behaviour for default image size because something new that is being added had a different default. That's a good sign something isn't right. |
…nt/resolution-option-for-featured-image-media-text-block
|
Good day, @aaronrobertshaw I apologize for my lack of explanation.
This takes care of that. If it is left undefined, the original image URL will be used. The same goes for large sizes as previously implemented. I prefer
I suppose this is why |
I appreciate the consistency across blocks angle but there is a commitment to backwards compatibility. If we're changing the end result of the save function, for instance due to changing the image src url, that might require a deprecation. On that note, have you tested blocks created before this PR on trunk and ensured they are still valid after the update to
There does appear to have been a valid reason for the existing behaviour. This is why Chesterton's Fence is such a great concept to keep in mind. |
But this is something that possible from theme.json now, we have setting for
Yes, i have tested it, Here what i did,
ill update the PR description. |
…nt/resolution-option-for-featured-image-media-text-block
I take it you mean possible via block editor settings? In which case, I agree 👍 Your recent update now provides the original behaviour of falling back to |
yes, that one. |
There was a problem hiding this comment.
Thanks for the persistence here @akasunil 👍
There still seems to be a gap in the fallback logic around the featured image size.
If I select a featured image that does not have the default large size it falls back to the thumbnail option in the UI but the image doesn't match that.
Screen.Recording.2024-12-09.at.10.21.56.am.mp4
Similarly, if you have a featured image selected and set the resolution to large or any size really, then switch the featured image to one that doesn't have that size, the resolution control again displays the thumbnail size as selected.
Screen.Recording.2024-12-09.at.10.25.34.am.mp4
FYI, I'll be AFK for most of the next few weeks. It might be a while before I can revisit this sorry. Feel free to ping other reviewers for fresh eyes and to keep the momentum going!
…nt/resolution-option-for-featured-image-media-text-block
…nt/resolution-option-for-featured-image-media-text-block
|
Thank you @aaronrobertshaw for your feedback.
I fixed this for normal images, missed to check for featured image. Anyway, this is fixed now for featured images too. |
There was a problem hiding this comment.
@akasunil the issues flagged in my last review, to do with having the correct size preselected for feature images etc, don't appear to have been resolved.
Some simple steps to replicate are:
- Add media and text block
- Add featured image that has
largesize - Select media and text block and choose to use featured image
- Select the
largesize for the image in that block - Change the featured image for the post to one that doesn't have the
largesize - Reselect the media and text block, switch to the block inspector and note the size incorrectly says
thumbnail.
Additionally, there are still other quirks like the featured image in the block defaulting to a different size than normal images. Shouldn't those be consistent?
- Add two media and text blocks
- Select a featured image for the post that has a
largeandfullsizes - Select the first media and text block and choose for it to display the featured image
- Note that this block's image defaults to
Full size - Select the second media and text block
- Choose to use an image from the media library and select the same image as used for the featured image
- Note that this image uses the
Largesize by default.
Did you test these after my last review? If so, are you seeing different behaviour?
Screen.Recording.2024-12-16.at.6.16.04.pm.mp4
It might be a while before I can revisit this sorry. Feel free to ping other reviewers for fresh eyes and to keep the momentum going!
I am out of bandwidth to keep reviewing this PR, sorry 🙏
To help other reviewers, with fresh eyes, it would help to keep the PR test instructions up-to-date. In particular, add some instructions around testing switching between images with different available sizes.
What?
Resolution size support for the featured image is missing in the media text block.
Why?
When the featured image is used in media text block, the
ResolutionToolis not currently available.How?
This PR implement
ResolutionToolsupport for featured image. Along with that, cleaning up some issues/bugs that was already present trunk and discovered while working on this PR.Following are the fixes,
Use Featured Imageon click of theResetoption in the toolbarUse Featured Imageoption on selection.Testing Instructions
Use Featured Imageoption from toolbar of Media-Text blockResolutiondropdown in Media-Text block controlsCheck the backward compatibility,
Screenshots or screencast
Edit-Post-.Hello-world-.-.-gutenberg-.-WordPress.4.webm