Layout: Add default fallback gap value in block.json, and set to 2em for Columns blocks #41098
Conversation
andrewserong
added
the
[Type] Bug
andrewserong
changed the title
|
Size Change: -2.25 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
andrewserong
changed the title
andrewserong
requested review from
TimothyBJacobs,
spacedmonkey,
ellatrix and
ajitbohra
as code owners
| @@ -33,6 +33,9 @@ | |||
| "padding": true, | |||
| "__experimentalDefaultControls": { | |||
| "padding": true | |||
| }, | |||
| "__experimentalDefaultValues": { | |||
There was a problem hiding this comment.
I wonder if it makes sense to use the proposed block.json changes from https://github.com/WordPress/gutenberg/pull/34180/files#diff-dba084b81a05295345ce9149af1a17ab073ddef43a393e851bcd3bdae85d3f32R81 for default values. (PR: Prototype: merge block CSS with theme.json styles #34180)
I don't know how far that particular investigation will go, but it could save some lines to "join forces"
cc @oandregal
There was a problem hiding this comment.
Oh, neat idea!
There's quite a lot of similarity here — for the case of blockGap we only want to use the value as a fallback value if none is set anywhere else, rather than always outputting it, so that might be a subtle difference between a default / fallback value and a value set in blockStyles. But keen for feedback or ideas on how to combine things if you have any @oandregal!
I suppose the caveat here is that this PR will likely need to be backported to 6.0 RC whereas it looks like #34180 is a bit more forward-looking and something potentially for 6.1?
There was a problem hiding this comment.
I suppose the caveat here is that this PR will likely need to be backported to 6.0 RC whereas it looks like #34180 is a bit more forward-looking and something potentially for 6.1?
Ah, very good point.
__experimentalDefaultValues does also do what it says on the tin as you say.
There was a problem hiding this comment.
Why not just blockGap: { default: "something" } instead of blockGap: true? (We do these object configs for a lot of block supports already)
There was a problem hiding this comment.
The individual feature properties of the spacing support are currently boolean | array and the array is treated as a list of sides that are opted into, with logic like the following function (and the below useIsDimensionsSupportValid:
It'd be a more complex change to refactor those props to also support an object (with a nested sides key), so I thought using a parallel object to set default values might make for a cleaner change given that this PR could be backported. There's a precedent in the Layout support, too, which has a default object and then within it, it specifies the properties:
We could always rename this from __experimentalDefaultValues to default, and it'd look like the following:
"spacing": {
"blockGap": true,
"margin": [ "top", "bottom" ],
"padding": true,
"__experimentalDefaultControls": {
"padding": true
},
"default": {
"blockGap": "2em"
}
},
What do you think?
There was a problem hiding this comment.
Also we'd have to find a model to define default side values too, right? Well, maybe not initially. Still that would involve some data massaging I suppose.
"margin": [ "top", "bottom" ]To something like
"margin": { default: { "top": "1em", ... } }🤷
The individual feature properties of the spacing support are currently boolean | array and the array is treated as a list of sides that are opted into
It's possibly not the cleanest way, but I guess we could check for the existence of a default key (?) if we had to?
if (
! support ||
typeof support[ feature ] === 'boolean' ||
!! support?.[ feature ]?.default
) {
return;
}I'm not saying it's pretty, and I kinda also wonder if the complexity is worth it.
There was a problem hiding this comment.
There's a precedent in the Layout support, too, which has a default object and then within it, it specifies the properties:
Yeah, that's the one I was thinking about, since you can also do. __experimentalLayout: true (a defined object is the same thing as "true"). I don't think we should bother changing the others (padding, margin) now, but would be good to decide on an approach. I'd vote for the object approach personally but wouldn't mind the alternative used here.
There was a problem hiding this comment.
Thanks for the feedback here! It turns out it wasn't as complex to deal with as I'd thought given that we only appear to look up which sides are opted into in the one place (useCustomSides).
In a2cc158, I've gone with the approach of:
"spacing": {
"blockGap": {
"default": "2em"
},
And we can also specify a sides key, too, in that object if we need to.
I'm trying think of other use cases for which this change would be useful. Basically, I'm keen on this PR and want to shore up my confirmation bias 🥴 but I can't right now 🤔 We're trying to set a style fallback value for a block; a value that can be referenced across the application where no other exists. Right now, it's the layout implementation that needs to know a particular block's gap value before it arbitrarily sets it own. The way the styles are output means that we can't solve this using CSS alone, e.g., by adding We want layout to overwrite such style when a gap value has been set by the user, but not necessarily when no gap value has been set by the user. It's this maze that has brought us here. So discounting a total refactor of [insert unknown code here], what this PR proposes seems to be the most straight forward approach. The main attraction for me is that we'll have a place to store Gutenberg block style fallbacks outside CSS files, removing the need for complex CSS overrides if we ever need to change other values in the future. I guess my only hesitation would be to extend the block API with something that is intended to address a special case. cc @gziolo asking you because I've seen your handle all over the block.json metadata 😄 |
| @@ -11,6 +11,7 @@ import { | |||
| arrowDown, | |||
There was a problem hiding this comment.
what about flow.js? no fallback support there?
There was a problem hiding this comment.
I noticed that too when I went to work on this feature — the Flow layout currently doesn't use any fallback value at all, so I didn't inject one where there isn't one already (both in layout.php and flow.js). From looking over the code again, it appears that blockGap styles in the Flow layout are only ever output if the blockGap has been opted into, in which case there'll always be a value, so a fallback value will never be reached. E.g. in the following block:
It was the Flex layout that currently has a 0.5em hard-coded fallback, and where I think this feature is useful based on the current implementations.
But, please do let me know if I missed something here! 🙂
|
Thanks for you thoughts @ramonjd!
Yes, that's my main hesitation too — whether or not it's useful beyond this special case. I guess that's also one of the reasons I leaned toward listing it as an |
|
This is working pretty well for me: Classic theme Block theme
I can't fault that logic. In some situations we need the freedom to explore ideas, which was part of the point of |
|
I did some testing manually and this appears to be working nicely. I know some people have strong feelings about putting |
|
Are we still trying to get this into 6.0, or are we looking at 6.0.1? This is basically the last item on the 6.0 Project Board, but since RC3 is already out, I was planning to move to the 6.0.1 board. However given the impact of the original issue and all the activity here, I'm second guessing myself. 😅 Just let me know either way, thanks! |
|
@peterwilsoncc highlighted this issue as one of the bug fixes we want to get into WP 6.0 RC 4 which will probably happen on Friday. See https://wordpress.slack.com/archives/C035NHLFUSY/p1652848301916789 (link requires registration at https://make.wordpress.org/chat). |
|
Perfect, thanks @gziolo! |
|
@andrewserong, do you plan to apply any feedback from the review or is it good enough as a temporary solution to ship in WordPress 6.0 next week? From my perspective, this PR can target directly |
There was a problem hiding this comment.
I've tested this in block themes, core classic themes going back to 2011, and a few custom themes e.g., Techine, OceanWP, Neve, Livro, Conceptly
this PR can target directly wp/6.0 branch and contain minimal changes that are scoped to the Columns block and have even specific flag in block support like supports.__experimentalFallbackDefaultGap: "2em".
This is interesting! So we'd add these changes for the wp-6.0 branch only?
Sounds like a neat way to give us some breathing room and finesse the API for 6.1. 👍
I suppose it'd make implementation in this PR simpler if we had a single block.json property __experimentalFallbackDefaultGap for columns block.json only, but would it make much a difference @andrewserong ?
…, updated useCustomSides to support an object
|
Thanks for the feedback, folks! I've updated the PR in a2cc158 to go with Riad's suggestion of moving the
I think the way it's implemented now, if folks agree, could happily merge into If there isn't consensus on landing the PR in its current shape, though, I'm happy to open up a separate PR targeting Thanks again! 🙇 |
|
I'm wrapping up for the week now, but feel free to merge this in, anyone, if the approach (and |
There was a problem hiding this comment.
Let's get this one in for WordPress 6.0 RC 4.
As discussed in the comment, we use __experimentalDefault to avoid introducing a new API just before the major WordPress release. However, it feels like it should be perfectly fine to make this API stable in a follow-up PR together with all the necessary documentation changes.
gziolo
added
the
Backport to WP 6.7 Beta/RC
…for Columns blocks (#41098) * Layout: Try adding a default fallback gap value in block.json * Implement in the editor, too * Try moving the default value to sit under the blockGap key in spacing, updated useCustomSides to support an object * Update doc comment * Add explanatory comments * Add __experimental prefix to default property. Co-authored-by: Adam Zielinski <adam@adamziel.com> Co-authored-by: Adam Zielinski <adam@adamziel.com>
|
I just cherry-picked this PR to the wp/6.0 branch to get it included in the next release: 497c7ec |
adamziel
removed
the
Backport to WP 6.7 Beta/RC
|
Please, can someone summarize what I should do to fix this issue? It's horrible to not be able to rely on Gutenberg ... now, if the sites auto update on Flywheel, they will look bonkers. |
Hi @carasmo! Do you mean the columns gap? This PR made it into WordPress 6.0 so the previous default of |
|
Thanks for the reply! I'm using 6.0 and there's no gap between columns. |
Hmm, I just took a look. I was testing WordPress 6.0 running Twenty Twenty-Two theme (latest version of Gutenberg plug not activated) and it looks like the new columns gap value is there:
Here's where the value is specified: https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/blocks/columns/block.json#L32 With the Gutenberg plugin activated however, and running WordPress 6.1-alpha-53448 and Twenty Twenty-Two theme (using this repo's wp-env), the default columns gap value isn't coming through however 🤔 |
Withdraw that comment. I hadn't built the plugin properly ( So it looks fine to me @carasmo Which theme and version of the Gutenberg plugin (if any) are you running? Or is it WordPress 6.0 only? |
|
I'm running a custom theme with 6.0 only. |
Sorry, I can't reproduce this 🤔 I'd be great if you could create a new issue with a description of how to reproduce, plus a screenshot of what you're seeing and the name of the theme you're using. We can then move the discussion to a dedicated place seeing as this PR is merged. Here's a link to create a new issue: https://github.com/WordPress/gutenberg/issues/new/choose Thank you! |
|
Note: I don't have a class Figure A (WP 6): Figure B (WP 5.9.3): |
|
I just ran into this problem as well (while testing several sites before updating to them to 6.0). These sites use a custom classic (non-block) theme and have a couple of custom dynamic blocks which output markup such as where the custom block relies on the styles that WP < 6.0 included in My current plan is to update the theme to include as a short-term workaround so that I can get the sites updated to 6.0. |
|
Yes, I'm going to have to go into many sites and add something in the additional css to address this. In the current site I'm working on, I'm doing the following. Soon I'm going to make my own column block and avoid these time suckers. |
mburridge
added
the
Needs Dev Note
|
Added the Needs Dev Note label in case this needs a dev note for the 6.1 release. |
andrewserong
added
the
[Feature] Layout
What?
This is an exploration into a potential fix for the gap regression in #40952 (it doesn't address the tablet breakpoint change raised in that issue).
In this PR we look at:
block.jsonwhere blocks can declaratively provide a default fallback value for properties of the spacing block support. This PR only implements support for theblockGapproperty.2emto be consistent with backwards compatibility.0.5em.The idea here is that this new fallback value isn't in
theme.jsonbecause it's a part of the block's styles — not the theme's, and is intended to form part of the base styling of the block, upon which themes can make their own changes.Kudos @ramonjd for collaborating on the idea in this PR!
Why?
In #40952 it was flagged that the Columns block used to have a default gap value of
2em. This PR explores seeing whether we can declaratively set the fallback value for blocks in theirblock.jsonfile — outside of the concept of theming. The gap of2emwas originally removed in #37436 when the Columns block was updated to opt-in to the Layout block support — it's the Layout block support that provides the default value of0.5em, so this PR gives the Layout support a little more power to look up and output a fallback value that is suitable for the block being rendered.How?
defaultprop to the features of thespacingobject inblock.jsonto specify a default fallback value. E.g.spacing.blockGap.default.2emto the Columns block, with a default fallback of0.5em.Testing Instructions
0.5em. With this PR applied it should be2emin both the editor and on the front end of the site.Screenshots or screencast