Cover Block: Add Image Resolution options #62926
Conversation
akasunil
added
[Type] Enhancement
|
Size Change: +500 B (+0.03%) 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? |
This is correct. I also think that the "Resolution" should be in the Settings panel, but since the
This is something to think about carefully, because For example, imagine a theme that doesn't have Perhaps a solution would be to enable background support by default only for the Cover block, but this would create another problem: it would override the root-level background support. A similar issue was reported in this comment. This is similar to how the Either way, I think the best thing to do at this point is to refactor the Settings panel to use #57540 might help. |
|
Thank you @t-hamano, I had already started with implementation. But, after seeing the issue that @richtabor had created, I got concerned that I might have to close this PR. 😄 Now that you have explained it, I'll continue with development. Thank you for reference PR. |
|
@t-hamano could use your review here. |
There was a problem hiding this comment.
Sorry for the late review. I left some feedback but overall it works fine 👍
| Before | After |
|---|---|
Also, I noticed while reviewing this PR that it is necessary to consider when multiple blocks are selected. At the very least, the alt text should be hidden when multiple blocks are selected. (Example of the Image block) This can be addressed in a follow-up.
akasunil
changed the title
There was a problem hiding this comment.
Thanks for the update! Everything works as expected, but I think we need to address one last issue. Imagine the following scenario:
- In the trunk branch, insert a Cover block and apply a background image to it.
- Check out this PR
- Resolution option is set to "Thumbnail"
What we expect here is that the Resolution option should be "Full size", just like when we insert a new cover block and apply a background image to it in this PR.
Perhaps we need a fallback default value, like with the Media and Text block:
I was puzzled why this piece of code in the mediaText block was there; I couldn't find a use case for it. Now that I know, I'll do the same with cover block. |
|
@akasunil Thanks for the update! I thought this PR was ready to ship, but after some more thought, I came to the conclusion that a block deprecation is needed. Typically, when a block with an image is saved, a This slug works as expected, but it doesn't take into account the Cover block already in the content. This means that we will need to either reselect full size from the Resolution option or reselect the image in order to update the markup to include the To address this, I think we need to add a block deprecation and add Have you ever implemented a block deprecation? Another question I have about this is whether or not we should include the gutenberg/packages/block-library/src/cover/save.js Lines 133 to 147 in 18c3e93 I may not fully understand the block deprecation, so let me send a ping to @aaronrobertshaw and @andrewserong 🙇♂️ |
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 |
What?
Fixes: #58819
Why?
Resolution option were missing from cover block for background images
While working on implementation of Resolution option in cover block, Its appeared that
ResolutionToolscomponent is composed ofToolsPanelItem, it must be placed inside theToolsPanelcomponent. The current Settings panel does not use the ToolsPanel component, so the entire settings panel is refactored.How?
This PR add
ResolutionToolto cover block, when cover has background imageTesting Instructions
Screenshots or screencast
cover-block-image-resolution-control.webm