Block Gap: Fix block spacing control for axial gap supported blocks #66783
Conversation
This file contains hidden or 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
Fix issue affecting blocks adopting axial block gap support when theme sets shorthand string value.
|
Size Change: +7 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
andrewserong
reviewed
Nov 6, 2024
There was a problem hiding this comment.
I only had a quick change to take this for a spin, but it's testing nicely for me so far! If there's still time, this feels like a nice fix to get in for 6.7 to me, and good idea treating the string-based value as a single value that we then pass along to an object representation of the gap 👍
I'll give it another test / review tomorrow if this it still open by then.
|
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. |
|
Thanks to everyone for their work here. I trust you'll understand that I've taken the liberty of removing the Backport label as there is no guarantee this can be included in 6.7. The final RC3 has been cut and if any final fixes are pushed to the 6.7 release they need careful discussion and approval from release leads. I appreciate your understanding here. From the Issue I note that this problem has been evident for some time:
Therefore I'm proposing that this fix does not meet the criteria for inclusion in any final RC prior to release. It also doesn't appear to be a critical PR that would significantly impact lots of sites. Please advise if I'm wrong. As a result I think it should be punted to 6.7.1 but I'd appreciate a confidence check from @kevin940726 @ndiego and @colorful-tones. |
|
@juanfra @carolinan Does this issue significantly impact TT5? |
|
I have not seen this issue, so not significantly? It depends how much trouble it is to add it this late. If there is an RC later today then maybe it can be included? |
Yes. I am all for fixing this issue asap, but we are running out of time on 6.7. If this issue was not introduced in 6.7 and does not directly impact TT5, it should be punted. |
This is my logic also. |
|
It is testing nicely for me. I believe the issue doesn't directly impact TT5. The fix will be a nice improvement for parity between the editor and the site. However, I also agree that as it wasn't introduced in 6.7, we may want to punt it. Screen.Recording.2024-11-06.at.14.20.23.mov |
|
Thank you everyone for weighing in. I'm happy to defer to the consensus of punting this to a point release.
To be clear though the issue this PR fixes does directly impact TT5. You can see where TT5 assigns a string value for So, yes, the bug wasn't introduced in 6.7 but TT5's use of shorthand string values exposes the bug more prominently in this release.
The emphasis on significantly here is the key. It definitely isn't critical but I don't think it's accurate to say the bug doesn't directly impact TT5. Hence this long-winded comment for anyone coming to this issue post-release 🙂 |
+1 |
ramonjd
approved these changes
Nov 7, 2024
andrewserong
approved these changes
Nov 7, 2024
There was a problem hiding this comment.
This is testing nicely for me and I haven't been able to fault it:
✅ Confirmed the issue in TT5 (or TT4 with the Buttons block)
✅ With this PR applied, for Axial controls, they're now consistently available in global styles when a string-based value is used, and making adjustments using the block spacing controls works as expected
✅ Blocks that do not use axial controls (e.g. Cover) continue to work as expected
From my perspective this would be a nice-to-have if there's another RC happening, as it fixes a bug that's present in TT5. While adjusting block spacing on particular blocks isn't a particularly visible bug in common usage (i.e. I imagine most people won't be adjusting this), for folks that do go to make adjustments this bit of the UI will likely feel broken without this fix.
That said, given how soon 6.7 is to coming out, punting to 6.7.1 also sounds reasonable to me, if we're comfortable with this being a known issue with the TT5 theme / GB behaviour.
LGTM!
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…ordPress#66783) Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: ndiego <ndiego@git.wordpress.org> Co-authored-by: juanfra <juanfra@git.wordpress.org> Co-authored-by: ltrihan <ltrihan@git.wordpress.org>
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.
Fixes: #61849
What?
Fixes issue affecting blocks adopting axial block gap support when theme sets shorthand string value.
Why?
Previously, when a block adopting axial block gap support had a simple string value assigned to
blockGapwithin a theme, the string value wasn't handled correctly.The block spacing control would expect a full block gap value object but instead get a non-axial value (an object with only
topdefined). This resulted in incorrect styles being generated where the "single" value was assigned to the row gap and0to the column gap. Any adjustment of the theme's value in Global Styles would break the gap styles.How?
Make the conversion of simple string values to a gap value object, aware of whether the block has axial support or not.
Testing Instructions
For testing you'll need a theme that assigns a string value to
blockGapwithin a block type's styles. The easiest option will be to test with TT5 as it does this for both the Columns and Buttons blocks."blockGap": truewithin their block.json supports.blockGapstyles to use different values e.g. simple strings, presets, or an object specifying different gaps per side (topandleft)In case it helps, here's a snippet of block markup covering multiple gap supported blocks
Screenshots or screencast
Screen.Recording.2024-11-06.at.4.59.25.pm.mp4