Cover Block: Image size option for featured image #67273
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akasunil
added
[Type] Enhancement
|
Size Change: +58 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
I have tested the pull request:
Are there any more specific test cases? |
|
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. |
|
|
If this works out well, I intend to provide featured image resolution support in |
aaronrobertshaw
approved these changes
Dec 2, 2024
There was a problem hiding this comment.
Thanks for for the follow-up here @akasunil 👍
This is testing as advertised for me.
✅ Selecting to use featured image via block toolbar works
✅ Application of feature image size is consistent between frontend and editor
✅ Switching between feature image and uploaded or media library image maintains resolution selection, including when done via Post sidebar
❓ One quirk I noticed was the resolution's impact on the focal point control above it. Selecting the thumbnail resolution causes some noticeable layout shift due to the size change meaning a scrollbar possibly getting shown/hidden.
That last one might be something to see if it could be ironed out in a follow-up.
I don't expect many real world use cases where the user wants to select thumbnail resolution for a background image on the cover block. I understand why it is in the available options, I wonder whether it should be though 🤔
One other very minor thing to note is the new featuredImage prop on the CoverInspectorControls. Could this cause some confusion? Earlier in the component, the featuredImage entity is retrieved under that name, only for the featured image's media to be passed to a prop called featuredImage later?
I don't have strong opinions on that naming but thought I might mention it in case others have better ideas.
For me, this change test well. I think we can address anything else needed in a follow-up.
Screen.Recording.2024-12-02.at.12.56.01.pm.mp4
Update featured image from post setting sidebar, and make sure cover background get updated in editor and on front-end with same resolution that was previously selected
@akasunil this should be added to the PR description's test instructions.
Every PR's description effectively documents the history of the project. It's important to make life as easy as possible for people needing to debug regressions etc. To that end, it helps if the PR description contains accurate information on how, what, and why. It's hard to predict how long discussion on a PR might go on for, so we shouldn't assume someone debugging regressions can, or will, read through it entirely to pull out missed test instructions.
I noticed this too but it already happens on trunk. |
Couldn't agree more, I'll keep this in check. Thanks @aaronrobertshaw and @carolinan for invasting, ill merge this PR. |
akasunil
deleted the
enhancement/resolution-option-for-featured-image-cover-block
branch
Apologies i have overlooked this, Should i change |
I don't think it's a big deal. It can always be cleaned up in a future code quality PR if others feel more strongly about it. |
im3dabasia
pushed a commit
to im3dabasia/gutenberg
that referenced
this pull request
Dec 4, 2024
…ess#67273) Co-authored-by: akasunil <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: carolinan <[email protected]>
michalczaplinski
pushed a commit
that referenced
this pull request
Dec 5, 2024
Co-authored-by: akasunil <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: carolinan <[email protected]>
|
Why doesn't this setting work in a page/post template? It only seems to work on a page where an image has been assigned? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Implement resolution size support for the featured image in the cover block, as originally discussed in #62926 (comment)
Why?
When the featured image is set as the background in the cover block, the
ResolutionToolis not currently available. The same is true for other media blocks (such as the featured image block), but we will only focus on the cover block in this PR.How?
This PR implement
ResolutionToolsupport for featured image.Testing Instructions
Use Featured Imageoption from toolbar of cover blockResolutiondropdown in cover block controlsScreenshots or screencast
Edit-Post-.Hello-world-.-.-gutenberg-.-WordPress.webm