Cover Block: Add Image Resolution options #62926
Conversation
akasunil
added
[Type] Enhancement
|
Size Change: +385 B (+0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
akasunil
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @Bladed3d. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
|
@t-hamano could use review here. |
|
If we do this, it should go in the "Settings" panel, like the Image block does. |
|
Flaky tests detected in 7a50d21. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10512593025
|
|
@richtabor If we have to transfer I see you've raised an issue (#64703) to provide background support to the cover block as a popover, like we do with the group block. If we're going to incorporate it in the cover block, I believe we should also include a resolution option in the background support. What do you think? |
There was a problem hiding this comment.
Thanks for the ping 👍
I'm still trying to understand the full intent behind some of the changes here so let me know if I've misunderstood something.
size-{slug} classes
What's the reasoning behind adding these?
- From what I can tell, these classes aren't used in relation to the display of an image of the requested size.
- The only references I could find were in old default themes (e.g. < TwentyFifteen)
- Could the sudden inclusion of these semi-generic classes break older themes?
- Are these classes only being added for "parity" with the Image block?
- If so, I don't think that's achieved given the Image block applies the class to the
figurewrapper and this PR applies it to the Cover block's innerimg.
- If so, I don't think that's achieved given the Image block applies the class to the
- Do we actually need the
size-{slug}classes? - These classes aren't added to the block in the editor. It should be consistent with the frontend
Deprecations
If the decision is that the size-{slug} classes are required in the markup, another call needs to be made on whether to enforce the "default" size class i.e. should blocks without a value in sizeSlug also get the size-full class.
If we enforce a class for the default image size, then yes a deprecation will be needed. If we don't enforce the default size class, then technically we might be able to avoid a deprecation as they are only required when an old set of attributes would generate invalid markup or when those old attributes need to be migrated at all.
Having said that, It is worth considering if avoiding a deprecation could create a "gotcha" down the line when someone else comes to update this complicated Cover block without knowing about the sizeSlug changes here. There is a performance cost adding deprecations so there are pros and cons.
Managing PR's scope
A final thought worth sharing is, perhaps we could split the refactor of the Cover controls to use ToolsPanel into a separate PR?
It would help better document the history of the block, make debugging in future easier etc. This is probably even more important if the scope of the PR expands due to needing to define a deprecation.
What do you all think?
|
Nice work here, it's testing well for me so far. Also a +1 for what Aaron mentioned, I'm curious about the need for the additional classnames. Sometimes when we have a need for additional classnames or deprecations I think it's worth taking a moment to see if there's another approach — the Cover block already has 13 deprecations that we need to maintain indefinitely so there's a bit of complexity here to wrangle. Also, with the HTML tag processor available, is there an opportunity to dynamically inject classnames when blocks are rendered, if that would avoid the need for a deprecation? I also see the feature isn't available when using featured images, where we'd need a server-side approach, too (should we wish to implement that in the future).
That sounds like a good idea to me, the more complex the Cover block gets, the harder I find it to review any changes to the block as it can be tricky to untangle if something isn't working correctly. Another thought: this adds the feature as an ad hoc control as part of the Cover block only. Now that background image support is being used more widely across other blocks (Group, Post Content, Quote, etc), will it feel odd if there's control over the background image resolution in the Cover block but not those other blocks? This isn't an argument for not proceeding with this PR, but I mostly wanted to flag that if we go with an ad hoc control, we'll likely still need to rebuild it in a unified way in order to support other blocks, so just thought I'd mention it in case it affects how we're designing this feature. |
I agree with you, ill create separate PR for refactor of the Cover controls |
I'm not entirely sure about this, i was following #24795 PR. Classes doesn't make any difference on front-end though. Also, I have split the PR for refactor of setting pane in cover block. |
|
@aaronrobertshaw @andrewserong Thank you for the feedback!
In my experience, if any media was used in the block, the class names Here is one purpose of this class that I could find. It seems to be used to apply styles only to images uploaded via WordPress. If this class is not necessary for the Cover block, we should not need to add the deprecation.
Yes, we did not expect the refactoring initially, but we needed to use
I think this is a very good question. If I compare the background image support with the ad-hoc control of the Cover block, the only thing that background image support is missing is the resolution option, and the functionality is almost the same. And considering the performance of the website, background image support should also have a resolution option. One concern is that the cover block supports the featured image, and if the featured image is used, background image support itself needs to be disabled. As far as I can tell, there is no way to dynamically control whether block support is enabled/disabled based on a specific block's attributes. Ultimately, is the best solution the approach of refactoring using a |
|
Could use your review on #65432 |
There was a problem hiding this comment.
Thanks for your patience here @akasunil 🙏
The current Settings panel does not use the ToolsPanel component, so the entire settings panel is refactored.
When you get a chance could you update the PR description to reflect the current state of the PR?
In the meantime, I've finally had a chance to give this another look after #65432 has been merged and this PR rebased.
It mostly works well except for some issues around resetting the new resolution control. I've left an inline comment on a thread to follow to fix it.
Screen.Recording.2024-10-18.at.12.21.35.pm.mp4
| // Try to use the previous selected image size if its available | ||
| // otherwise try the default image size or fallback full size. |
There was a problem hiding this comment.
| // Try to use the previous selected image size if its available | |
| // otherwise try the default image size or fallback full size. | |
| // Try to use the previous selected image size if it's available | |
| // otherwise try the default image size or fallback to full size. |
Nit: Typos
| @@ -219,6 +256,7 @@ export default function CoverInspectorControls( { | |||
| </ToolsPanelItem> | |||
| </> | |||
| ) } | |||
|
|
|||
There was a problem hiding this comment.
Nit: Accidental whitespace change after rebase?
| <ResolutionTool | ||
| value={ sizeSlug } | ||
| onChange={ updateImage } | ||
| options={ imageSizeOptions } | ||
| /> |
There was a problem hiding this comment.
The reset behaviour for this control seems pretty odd.
I think it might be missing the defaultValue prop otherwise it shows as having been modified before the user touches it. It's also used when resetting the control individually.
If the default value prop isn't provided it will fall back to the first option in the available sizes which in my testing was thumbnail. That is different than how this panel's resetAll is resetting the sizeSlug.
There was a problem hiding this comment.
It should be working fine now, i have set default value to "Full Size"
Edit-Post-.Test.-.-gutenberg-.-WordPress.webm
|
Thank you @aaronrobertshaw for quick review.
I have updated the description. |
There was a problem hiding this comment.
Thanks for sticking with this one @akasunil 💪
The resetting behaviour now works for me after the latest updates. I think this is good to go 🚢
Screen.Recording.2024-10-21.at.12.17.44.pm.mp4
| const { gradientValue, setGradient } = __experimentalUseGradient(); | ||
| const { getSettings } = useSelect( blockEditorStore ); | ||
|
|
||
| const imageSizes = getSettings().imageSizes; |
There was a problem hiding this comment.
It appears as though there are a large number of places in the codebase where the presence of imageSizes in the settings is assumed. This could still be a touch more robust in the face of future changes if we fell back to an empty array if it isn't available. Up to you if you'd like to tweak it before landing this.
There was a problem hiding this comment.
If imageSizes is set to empty, ResolutionTool wont be visible. We could set fallback value for this, but if user have set imageSizes setting to empty array, should we fallback it to default value again?
Also, We could create hook to set default value if imageSizes setting is empty.
There was a problem hiding this comment.
If it is null or undefined, the filter and map call below would throw an error.
There was a problem hiding this comment.
I have added optional chaining operator to avoid errors. Thanks for the feedback.
|
I'm curious why the resolution setting isn’t available when a feature image is used. Are there specific reasons for this limitation? |
|
Yes, we really need this for featured images. It seems to have been missed and is using the full size. |
Featured images work differently, so they likely need specific handling. My understanding is that this was outside the scope of this PR. With the complexities encountered here, it makes sense to refine that and ensure the approach is solid before expanding the scope to featured images as well.
If there isn't an issue already dedicated to this, it would be great if you could create one. Having a dedicated issue creates awareness and helps any contributor that is interested to jump in and propose fixes and enhancements. |
|
It looks like this is the dedicated issue: |
* Add attribute for resolution of image in cover block * Add resolutionTool into cover block for background images * Relocate the resolution control in inspector control * Add image size class to image on save * Retain previous value of sizeSlug attribute on image change * Update url based on selected image size * Update setting panel using ToolsPanel component * Remove clear media button from setting panel * Remove Resolution Tool component from dimension panel * Update role of setting panel in unit test * Remove unneccessory code * Remove typecasting for boolean variable and update default value * Set default value to full size for media size slug attribute * clear whitespace and typos * Set default value for image resolution * Fix empty image sizes condition Unlinked contributors: Bladed3d. Co-authored-by: akasunil <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: codebymikey <[email protected]>
Hi @aaronrobertshaw, Maybe I'm missing something, but isn't it just a matter of passing the |
What?
Fixes: #58819
Why?
Resolution option were missing from cover block for background images
How?
This PR add
ResolutionToolto cover block, when cover has background imageTesting Instructions
Screenshots or screencast
cover-block-image-resolution-control.webm