Columns block: Enable blockGap and vertical margin support #34630
Conversation
andrewserong
added
[Type] Enhancement
|
Joen, I've added you as a reviewer since we've both been working on gap support, please let me know if there are other folks I should ping for feedback, too! |
|
Always happy to review 👌 Nice work on this one. I like that it's a minimal change to the codebase. More than anything this illustrates the need for some thought put into the sidebar panels. We've been talking about Dimensions, Spacing and Layout panels for a bit now, and waffling on which controls go where, and whether perhaps we have only two of the above. For example, gap might be a layout control since it affects the items inside. But then isn't spacing? And if we do have spacing, couldn't block gap be part of that? I know this is something @youknowriad has thought a lot about as well, would love his input. |
Totally agree, and something I started highlighting in #34558. I feel we could condense most of the dimensions type controls into a more global Layout panel, but the tricky part is, that while the Group block has a "Layout" panel, the "Columns" block does not. Comes down to the idea that perhaps we have a Layout panel that adapts a bit based on the block. For example, perhaps the Group block's layout panel has the "inherit default layout" and "alignments" parts, as well as blockGap — while the Column's block layout panel has the columns count UI, stack on mobile, and blockGap. I'll relay these thoughts on my original issue, but wanted to chime in here. In other new though — I tested the PR and it works great! :) |
andrewserong
force-pushed
the
update/columns-block-to-use-gap-block-support
branch
from
99625bd
to
fac8026
Compare
There was a problem hiding this comment.
Tested this and it's working as expected for me. I also agree on thinking about where these settings go.
I'm also concerned about calling this "Block gap" as a setting in this context. I know with the supports mechanism this is difficult, but would it not be better named contextually in situations like this? So calling it "Column gap" here? Not a blocker obviously.
|
Thanks for testing @apeatling!
That's a great point — I suspect if I were a new user, I'd have no idea what "Block gap" means, but "Column gap" or ("Image gap" in the context of a Gallery block) would be much clearer. I think it'd also help a bit with the "Block gap" control feeling possibly out of place in the Dimensions panel as it is now. With a better name, maybe it'll feel slightly more at home. In some of the other designs folks have shared (e.g. Joen in this comment #28356 (comment)) I see that the label "Block spacing" has been used, which might be a bit more natural for users? I really like the suggestion of "Column gap", though, so my preferred option would be to see if we can allow each block that opts-in to the blockGap support to set its own override label (@aaronrobertshaw had a good idea of seeing if we can set this in the |
|
Nice work!
I like the fact that margin is available if only to override the I'd expect a grid block, with multiple rows to spread spacing evenly. Caveat: I'm probably missing context somewhere. With margin support:
Without:
Do you think it's worth making margin a default control in the panel? It's not a biggie. As others have mentioned it tests well otherwise!
Providing the ability to tweak labels in theme.json sounds like a flexible solution. I ask myself though if it will open the doors to JSON bloat. Gutter seems like a standard term to describe spacing between columns. I'm not sure if it's generic enough to speak to a wider audience though. It also might connote even distribution of space between column rows, which we won't have until we have a grid block 😆 |
|
Thanks for testing @ramonjd!
I believe that margin rule comes from the Layout block support styling via #33812. There's some discussion from this comment onwards on that PR that gives a bit more context as to the usage of top margins on child elements to control that particular spacing. As long as that CSS rule is present, I think it's worth us having the margin control in place (even if it's hidden in the menu) so that once we start adjusting the gap value, we can fix the margin in place if we need to. At least, that was my thinking!
Quite possibly! I think the idea is that if we can have it in Gutenberg's |
Block spacing is probably a great thing to start with, as it's agnostic across the blocks on which it's applied. At the moment, the columns block is decidedly desktop-first in its approach to a row of columns, but even now it has a "stack on mobile" option, where columns wrap onto new lines. In that context, and in light of potential intrinsic responsiveness (#34641), block spacing works to indicate it's meant both for vertical and horizontal spacing. "Column spacing" might work as well, but before we add extra conditionals it feels worth it to see if just "Block spacing" feels intuitive enough in use. |
andrewserong
force-pushed
the
update/columns-block-to-use-gap-block-support
branch
from
fac8026
to
5c0fb4c
Compare
|
Size Change: +16 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Thanks for the feedback @jasmussen! I was curious to see how we might go about overriding the label, because I was thinking about those other use cases for gap support that might not involve inner blocks (and to see how difficult it might be). I've updated this PR with an exploration: It seemed to be fairly straightforward to add in a couple of simple functions to use the To show what it looks like now, here's the view with the
One problem with
I'm keen to hear what everyone thinks, both for which string to use, and for what's an appropriate approach for overriding a block support's control label from a block's settings. I'm not at all attached to the approach that I included in this PR, so happy to go in another direction or revert as needed 🙂 |
|
Thanks for continuing on this. One instinct I have in exploring new ground is to always do as little as possible and leave as much as possible to followups. That way they can happen if they turn out to be necessary, whereas if we start with too much we don't realize what might have been unnecessary in the first place. In the case of gap, the precedence I keep coming back to is Figma's autolayout, which calls the feature "Spacing between items" — somewhat similarly agnostic to "Block spacing". So it's nice to know that tweaking the label is possible/easy, in case it becomes clear we really do need it. In the case of the available space, while the following mockup remains subject to change per details being worked on in #34574, it does point to the block spacing control always being a full row, rather than a 50% width column item:
In the above, the icon to the right of the slider both serves to indicate which spacing is being adjusted, and as a button to toggle axial controls. Followup territory of course, but it does suggest the full row of space is available. |
| @@ -79,6 +79,11 @@ export const settings = { | |||
| }, | |||
| ], | |||
| }, | |||
| __experimentalLabel( _, { context } ) { | |||
There was a problem hiding this comment.
This API sounds very weird to me, using a "block API" to label a block support. Did we consider a config in the block.json file?
There was a problem hiding this comment.
It is pretty weird, particularly now that I've had a chance to sleep on it! I think a config in block.json would be a better way to go about it.
I agree with Joen, though, that it'd be good to try to do the least amount of work here as possible for the Columns block, so in the immediate term, I'm going to revert the label overriding and just set the string to a hard-coded Block spacing for the time being.
If and when we get back to it, I'll take a look at seeing if we can set it in the block.json file. Thanks for taking a look 🙂
|
Thanks for the feedback @jasmussen!
That's very wise, and I agree, I really like small iterative PRs rather than packing in too much at once. I've reverted the label override behaviour, which we can defer to figuring out further down the track if we need it. I've retained the update to the string so that it now reads
Good to know! Since the label I explored removing the
Do let me know if you'd like further changes or tweaks, of course! |
|
I agree with Riad, let's test in the plugin first for a cycle. |
There was a problem hiding this comment.
The columns spacing + margin looks good to me. Copy change is also 👍
Oct-05-2021.12-24-44.mp4
✅ I can apply block spacing and margins in the block and site editors. The frontend appears as I'd expect and the styles are applied, e.g,
.wp-block-columns {
margin-top: 62px;
margin-bottom: 62px;
--wp--style--block-gap: 64px;
}
I noticed that I cannot completely clear the margin value from the field.
I've approved this PR as the changes aren't related it appears.
In the Block Editor the value persists until I remove the field or Reset All.
In the Site Editor it persists even after I've Reset All.
Is that expected?
I was just testing this again and it's only when I clear the field and blur out. If I hit the Enter key, the value persists. Maybe we need to handle null values in the blur event of UnitControl? I don't know yet 🤷 |
|
Thanks for testing @ramonjd! I think the different behaviour you're seeing with the I think I have a personal preference to not requiring pressing Enter to commit the value in the BoxControl, but that's probably something for us to look at in a separate PR? I can see an argument for going either way. |
Thanks for confirming! I was looking at mayUpdateUnit() and came across it. I wanted to see if we could do something differently
Definitely. I just wanted to flag it. I also have a personal preference (or maybe an expectation) that I can clear a form field's value this way, especially if the fields in the same panel behave differently. Not a blocker for this PR. ✅ |
Thanks for confirming 👍
Okay, great, since we both have the same expectation, I'm happy to look into a PR for the margin and padding controls so we can start the discussion of switching it off there. I'll merge this PR in now. If anyone thinks of other follow-ups we should look at, too, please let me know! |
|
We sometimes need to space elements away from the edges of their parent container, such as when using a full-width Gutenberg block. Will the user also be able to add E.g., the mockup in issue #24874
|
Is this the effect that you're describing?
To achieve the above effect, it's possible to turn left and right margin and padding support on in "spacing": {
"blockGap": true,
"margin": true,
"padding": false,
"units": [ "px", "em", "rem", "vh", "vw", "%" ]
},I guess it depends on the underlying layout styles to whether left and right margins will have any effect however. For example, the default layout width carries with it the following CSS: max-width: 650px;
margin-left: auto !important;
margin-right: auto !important;
|
Yes! Thanks for the examples too 😃 |
|
@ramonjd "settings": {
"spacing": {
"customPadding": true,
"customMargin": true,
"units": [ "px", "em", "rem", "vh", "vw" ]
}
}
The padding controls display and works correctly. However, I can't see the margin controls. Is there some additional setup to be done? P.S: I used |
|
@MuluhGodson, for clarification, what type of Gutenberg block settings do we see in the example above? It may be that padding is enabled for some blocks but not others? |
The settings above are applied globally to all block elements. So each block can have its own padding and margin controls. |
👋 @MuluhGodson Which block are you trying to target, and where is the JSON? I think @brylie has the answer: "It may be that padding is enabled for some blocks but not others?" Enabling margin and padding globally via So in your theme's "spacing": {
"blockGap": true,
"margin": true,
"padding": true,
"units": [ "px", "em", "rem", "vh", "vw", "%" ]
},This "enables" all spacing supports, and the blocks that opt into them via their individual block.json settings will show them. If the block doesn't opt in You can control individual block settings via So you could set the spacing controls to show for the core Group Block in "blocks": {
"core/group": {
"spacing": {
"blockGap": true,
"margin": true,
"padding": true,
"units": [ "px", "em", "rem", "vh", "vw", "%" ]
},
}
}More information on theme.json over here. Hope that helps! |
|
Should we be using |
Since #36155 |
|
|
Is there a way to limit the margin to predefined options? So instead of allowing any size value, there would be choices of say, small, medium, and large that are set in theme.json? This is how text sizing works, and with text, you have the choice to enter a custom value, but can also disable custom values being entered in theme.json. |
👋 This relates to the discussion over at #38998 about standardized tokens. |
Description
Following on from #33991 and the Columns CSS change by @richtabor in #34456, this PR opts the Columns block in to the gap block support control, and also the vertical margin control.
Adjusting block gap allows the user to adjust the space between columns blocks. Depending on where the columns block is placed, and whether or not the theme uses the Layout support, the adjusted block gap value can also change the gap between the selected block and blocks around it. To allow the user to tweak this, the margin control is added, so that the margin can be controlled separately to the gap on inner blocks.
Up for discussion: which controls are good to display by default? Does it make sense to expose blockGap by default, but hide margin? Should we default to both? I'd love to hear feedback on what's best to expose here.
Changes proposed
Block spacingHow has this been tested?
Running TT1-Blocks theme, in the
theme.jsonfile, undersettings.spacing, enable theblockGapandcustomMargincontrols. For example, here's how that section of mytheme.jsonfile looks:Screenshots
columns-block-gap-support.mp4
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).