Block spacing: block-level axial gap block support #37736
Conversation
|
Size Change: +243 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
Just jotting down notes. I have an open question about what to do with block-level theme.json properties. The gap value is currently available only at the top level and assigned to a CSS VAR in PROPERTIES_METADATA. We're turning on blockGap support in individual blocks – see Columns Block for example – so I would imagine theme authors might want a way to control things at the block level in Put another way: Should this toggle spacing block supports for an individual block? "settings": {
"blocks": {
"core/social-links": {
"spacing": {
"blockGap": true
}
}
}
},
Should this determine the default value of block spacing for an individual block? "styles": {
"blocks": {
"core/social-links": {
"spacing": {
"blockGap": "20px"
}
}
}
}
Also, even if we do allow global styles for blockgap, they're being overwritten by the CSS generated by layout.php support
|
ramonjd
force-pushed
the
try/block-split-axial-gap-support
branch
from
44a7a4f to
5a9bbcb
Compare
ramonjd
force-pushed
the
try/block-split-axial-gap-support
branch
from
5a9bbcb to
994102a
Compare
ramonjd
added
[Feature] Design Tools
ramonjd
requested review from
ellatrix,
tellthemachines,
mkaz and
ajitbohra
as code owners
ramonjd
requested review from
scruffian,
andrewserong and
glendaviesnz
and removed request for
mkaz,
ajitbohra,
ellatrix and
tellthemachines
There was a problem hiding this comment.
This is testing really well for me, @ramonjd, thanks so much for persevering with axial support!
Just left an optional comment about possibly moving some of the logic to a separate function to tighten thing up a little bit.
Overall I think this is a really clever approach — opting in to axial support for those blocks that use flex layout, where the gap value is used in the shorthand syntax, so concatenating the separate values together works nicely. This sounds like the kind of thing that we'd want to support long-term to me, but it might be worth confidence checking with other folks, too, that we're happy to rely on a (potential) implementation detail of the layout support? CC: @youknowriad in case there are any concerns there.
I really like the idea of starting out with this only supporting axial controls at the individual block level, and the global styles UI controls appear to be unaffected.
Tested with the social links, navigation, and columns blocks on a few different browsers, and using the TT2 theme.
The only really small issue I noticed (and I think this is to do with the BoxControl component's axial control support, rather than this PR) is that it doesn't say mixed when there's differing values (unlike, say, padding on the group block), but I don't think that's a blocker:
Otherwise, this is looking really good to me!
| @@ -28,7 +28,7 @@ | |||
| } | |||
| }, | |||
| "spacing": { | |||
| "blockGap": true, | |||
| "blockGap": [ "vertical", "horizontal" ], | |||
There was a problem hiding this comment.
Just a note that I think this is a deceptively powerful feature (by which I mean, very cool feature!) — because vertical spacing only affects the columns block on mobile viewports, this means that it creates a nice ability to be able to control desktop versus mobile spacing, without having to consider viewport widths directly 👍
There was a problem hiding this comment.
Which formats do we support here and what do they mean?
There was a problem hiding this comment.
Which formats do we support here and what do they mean?
I'm not clear what you mean here. Was it in relation to the above comment, or the formats of "blockGap" in theme.json (string|array)?
because vertical spacing only affects the columns block on mobile viewports, this means that it creates a nice ability to be able to control desktop versus mobile spacing, without having to consider viewport widths directly
This comment makes me think of a recent conversation on how blockGap effects margin-top via the layout abstraction. Namely, how can we reconcile the need for zero margins in a world where "blockGap" has a value in theme.json.
| if ( typeof next === 'object' ) { | ||
| const row = next?.top || 0; | ||
| const column = next?.left || 0; | ||
| newValue = row === column ? row : `${ row } ${ column }`; |
There was a problem hiding this comment.
Why are we storing the value as a concatenated string, shouldn't we store the next object as is like we do in other kind of properties (padding, margin...)? Is it because we only allow two sides? if that's the case should we instead use a { vertical, horizontal } instead if { top, left } is not good enough?
There was a problem hiding this comment.
It was out of convenience to use the CSS shorthand; mainly so I didn't have to alter the layout implemention.
There might have been other reasons, but I can't recall them. I'll revisit and confirm. 👍
I guess it might be better in the long run if the values were separate in case we want to use the values independently. Though ultimately the approach wasn't right, it's where we were trying to go with #35454, which added separate CSS vars for row and column.
We could do a check for is_array( $gap_value ) and, in that case, declare separate row-gap and column-gap values here.
Definitely worth another look, thanks for nudge.
if that's the case should we instead use a { vertical, horizontal } instead if { top, left } is not good enough?
{ top, left } should be good enough for our purposes for now.
The top, right, bottom, left prop names are baked into the Axial Input Controls in the BoxControl component anyway.
There was a problem hiding this comment.
Definitely worth another look, thanks for nudge.
Actually, definitely worth doing now that I've looked a little more closely. We need to support "blockGap": [ "vertical", "horizontal" ], and "blockGap": [ "vertical" ], for example.
There is no "doh!" emoji 😆
There was a problem hiding this comment.
Why are we storing the value as a concatenated string, shouldn't we store the next object as is like we do in other kind of properties (padding, margin...)?
523d9be is a very rough first stab at it.
I'm yet to address the question of the style model. It's made a smidge easier given that we're not adding gap inline CSS to block containers.
But if we have to match the CSS properties, I think we'd need to use the longhand properties.
setAttributes( {
style: {
...style,
spacing: {
...style?.spacing,
rowGap: next?.top || 0,
columnGap: next?.left || 0,
},
},
} );Which probably means changing a whole bunch of other references.
ramonjd
force-pushed
the
try/block-split-axial-gap-support
branch
from
994102a to
523d9be
Compare
ramonjd
requested review from
TimothyBJacobs and
spacedmonkey
as code owners
ramonjd
force-pushed
the
try/block-split-axial-gap-support
branch
from
523d9be to
7de1819
Compare
| const column = blockGapBoxControlValue?.left || defaultValue; | ||
|
|
||
| return row === column ? row : `${ row } ${ column }`; | ||
| } |
There was a problem hiding this comment.
Here I was wondering whether we should be returning the full definition so that we can isolate row and column values where only one is set, e.g.,
// row and column is set or is the same
return `gap: 10px 10px;`;
// only row is set
return `row-gap: 10px;`;
// only column is set
return `column-gap: 10px;`;That way, if a block ever has to inherit either row/column we're not overwriting it.
Just typing out loud...
There was a problem hiding this comment.
That would be a nice feature, though if we do it here we should do it on the server-side too 🙂
There was a problem hiding this comment.
After some further tinkering I probably need to put more thought into this.
I'm picturing a future where theme.json authors could add something like this.
"spacing": {
"blockGap": {
"row": "0.5em",
"column": "2em"
}
}and blocks would then inherit or use these values as defaults.
Currently "blockGap" accept a string, which can be really anything, including the shorthand value of "0.5em 2em" so it might take some more cogitating to make things backwards compatible.
There was a problem hiding this comment.
Sure, we don't need it for this PR; better to work out the details as a future enhancement.
ramonjd
force-pushed
the
try/block-split-axial-gap-support
branch
from
2bcb42b to
bb2612f
Compare
There was a problem hiding this comment.
Code looks good and functionality is working well 🚢 Just some minor questions/comments below. Also:
We're turning on blockGap support in individual blocks – see #37436 for example – so I would imagine theme authors might want a way to control things at the block level in theme.json as well.
This would make sense. We don't necessarily want to be using the same gap for something small and inline like Social Icons as we would e.g. for Columns, so themes should be able to set this.
Also, even if we do allow global styles for blockgap, they're being overwritten by the CSS generated by layout.php support
Could we somehow write the global style value to the --wp--style--block-gap variable? Or else perhaps the editor default should be at a lower level.
In any case, that can be addressed as a separate PR 🙂
| const column = blockGapBoxControlValue?.left || defaultValue; | ||
|
|
||
| return row === column ? row : `${ row } ${ column }`; | ||
| } |
There was a problem hiding this comment.
That would be a nice feature, though if we do it here we should do it on the server-side too 🙂
ramonjd
force-pushed
the
try/block-split-axial-gap-support
branch
from
bb2612f to
75ade72
Compare
There was a problem hiding this comment.
Thanks for persevering with this feature @ramonjd! This is testing well for me in the editor and on the front-end! 🎉 Since it looks like the blockGap isn't currently functional in Global Styles, would it be worth us removing / hiding the control there for the time being? (The fact that it doesn't currently do anything isn't because of this PR, of course)
I like the approach to concatenating the row and column values into the gap value for those blocks where we opt-in to the axial controls 👍. It's otherwise working really nicely for me!
|
Thanks for taking this for another spin @andrewserong 🙇
Just so I know we're talking about the same thing 😄 Do you mean for blocks?
This isn't having any effect for me either, and I've never noticed it before. So I totally agree! 😆 It's turned off for ROOT_BLOCK_SUPPORTS as well, so I don't see any harm in removing it from the dimensions panel. I'll throw up a PR. |
There was a problem hiding this comment.
LGTM, thanks for opening up the follow-up PR to remove the controls from the dimensions panel in global styles!
I suppose to make it neater before landing, we could remove the changes to packages/edit-site/src/components/global-styles/dimensions-panel.js?
… object. We're using the `top` and `left` properties of the BoxControl next value to represent row and column values. Reverting some block.json changes. Ensuring that we cater for blockGap string values. Reverting some block.json changes. Ensuring that we cater for blockGap string values. Adding tests.
…tests Added comment to explain how we build and pass the boxcontrol component value.
…n theoretically be something like "24px 54px" or some other manifestation of the shorthand value.
ramonjd
force-pushed
the
try/block-split-axial-gap-support
branch
from
0af1085 to
b618d4e
Compare
* Initial commit: splitting the gap value to allow for axial gap values * Checking for object. Now passing complete object to box control. * Providing a default value for split gap values, where one is `undefined`. * This is the first commit that switches the blockGap over to use to an object. We're using the `top` and `left` properties of the BoxControl next value to represent row and column values. Reverting some block.json changes. Ensuring that we cater for blockGap string values. Reverting some block.json changes. Ensuring that we cater for blockGap string values. Adding tests. * margin-top should take a single value * null coalescing operator (??) is not present in PHP * Checking for gap array items before using them. * Using `--wp--style--block-gap` as the fallback for split values * Returning null instead of the argument in getGapCSSValue and updated tests Added comment to explain how we build and pass the boxcontrol component value. * Rolling back using the gap CSS var as a fallback since the CSS var can theoretically be something like "24px 54px" or some other manifestation of the shorthand value.
Resolves #34529
Description
I'm not 100% sure whether this is the right approach. For individual block supports it seems to do the job. Just throwing it out there for now.
How has this been tested?
Using TwentyTwentyTwo, or another block based theme with
blockGapsupport enabled intheme.json.In social-links/block.json, switch the value of
"supports.spacing.blockGap"fromtrueto[ "vertical", "horizontal" ]With axial
i-have-friends.mp4
Without
i-have-friends-2.mp4
You can test with any appropriate block that supports
blockGap, for example Navigation. Make sure to enable the support in the block.json file as described above.Example editor code with blockGap as string value
The above code uses strings values for
"blockGap".When toggling them they should covert to the style object
{"style":{"spacing":{"blockGap":{"top":"1.13em","left":"1.13em"}}}}.Point being, we should handle both string and object values seamlessly for backwards compatibility.
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).