-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Buttons block: block spacing value does not apply to both vertical and horizontal alignment #64971
Conversation
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. |
Nice one, thanks for the PR! I'd appreciate an additional eye by @WordPress/gutenberg-design on this. One instinct might be that the available block spacing control should be contextual to the orientation of the block, e.g. when the block is horizontal, only show horizontal gap, etc. However, if you add enough buttons in the horizontal orientation, it wraps: To that end, I'd tend to think this is a good solution, showing both controls. These controls also sit in context of the layout container itself, which has room. Finally, there's likely an opportunity in the future to consolidate, so it may be a single gap control that you can split apart, like this: Kudos @jarekmorawski for that mockup. One thing to note from the issue reported: So in the mean time, this seems at a glance, ready for a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR makes sense to me too 👍
It's very useful to have separate control over horizontal and vertical gaps when blocks may wrap. The Social Links block already supports separate control too.
In the future, we may be able to improve the UI itself more, as suggested by @jasmussen.
I'll ping @andrewserong, @aaronrobertshaw, and @tellthemachines as I may have missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping, this LGTM, too and is testing well for me:
✅ Axial gap controls are working correctly at the individual block level in the post and site editors, and on the site frontend
✅ Global styles controls for the Button block work as expected
Agreed that the controls could be improved in UI follow-ups, though! Axial controls can add an additional step for folks that want consistent spacing in both directions, so it isn't always the easiest thing to use.
One thing to note if folks are testing with TT4 theme, is that it sets a single value for the Buttons block's blockgap: "blockGap": "0.7rem"
. As a result the UI defaults to a single control at the Buttons block level in global styles:
If we remove that value from the theme.json
file, then the axial controls are available:
I don't think that's a blocker for this PR, but just wanted to flag it as a potential gotcha and/or something to look into in follow-ups.
@andrewserong Thanks for the feedback! So let's merge this PR 👍 |
This is interesting. It seems to occur not only in the Buttons block, but also in other issues that support the axial controls. For example, if I define a theme.json like the one below, I can see the same problem: {
"version": 3,
"settings": {
"appearanceTools": true,
"layout": {
"contentSize": "840px"
}
},
"styles": {
"blocks": {
"core/social-links": {
"spacing": {
"blockGap": "1em"
}
},
"core/columns": {
"spacing": {
"blockGap": "1em"
}
}
}
}
} Perhaps this problem needs to be improved by investigating the DimensionsPanel component itself, |
Is that a reason to rewind this PR? Or can bugs be fixed in the post beta period? |
The problem below has been around for a long time, so I don't think there's any need to rewind this PR:
I tested it in the playground and I can confirm this issue at least from WP6.3. If this problem needs to be fixed, I think it needs to be fixed before the beta release. After the beta release, basically only bugs and regressions that occurred in the major version are allowed to be fixed. |
I intend to fix this issue by the time of the WP6.7 beta, and as a first step I have submitted #65287 to fix an uncommon bug. |
This change introduced an unintended bug to the existing control (single sided). See this comment Changing value applies to only I have done some testing of #65306 and it is fixing both issues. |
What?
Why?
How?
horizontal
andvertical
block gap options.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
GB-issue-64948.mov