Allow layout controls to be disabled per block from theme.json #53378
Conversation
tellthemachines
added
[Type] Enhancement
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/class-wp-theme-json-gutenberg.php |
| @@ -415,6 +415,7 @@ class WP_Theme_JSON_Gutenberg { | |||
| 'writingMode' => null, | |||
| ), | |||
| 'behaviors' => null, | |||
| 'layout' => null, | |||
There was a problem hiding this comment.
Layout already exists further up in this array, so I think this line will override that earlier entry?
Does layout need to be null at the root of this array to support it being set to false?
There was a problem hiding this comment.
If it's not in there it won't show up in the settings coming from theme.json, and when we get here the path is empty.
I don't think we have any other block supports that can be fully disabled by setting the whole thing to false but it would be a desirable feature to have for locking down style editing. Perhaps this needs to be looked at more holistically.
There was a problem hiding this comment.
Do we know what overriding the earlier entry breaks for that matter? Content and wide sizes still seem to work for me locally 😅
There was a problem hiding this comment.
Ah, gotcha!
Do we know what overriding the earlier entry breaks for that matter? Content and wide sizes still seem to work for me locally 😅
I don't think it'll break anything, as far as I'm aware. Just that now anything within layout will be treated as a valid setting (not just contentSize and wideSize). So long as the rest of our code is okay with that, I think that'll be okay, but it could be good to double check with folks that might have a deeper understanding of how this works. I'll just ping @oandregal in case he has any opinions when it comes to the settings.
In terms of this PR, I think it means we could likely keep the 'layout' => null, but we can also remove the above layout => contentSize, wideSize entry, since it gets overridden by the time the code executes? Here's what it's logging out for me (a single layout => null entry):
There was a problem hiding this comment.
I don't think we have any other block supports that can be fully disabled by setting the whole thing to false but it would be a desirable feature to have for locking down style editing. Perhaps this needs to be looked at more holistically.
Actually, locking down styles was the first thing that theme.json settings supported 😀 Examples are color.custom (disables custom color for users) or typography.custom (disables custom typographies for users).
useSetting was created to abstract all the places where this info can be sourced from: a block ancestor, the own block instance, settings (merged theme.json), etc.
So, instead of creating a new key and accessing it directly, the recommended way would be using a sub-key of the existing layout key + accessing it via useSetting.
There was a problem hiding this comment.
If we do go with
enabledthough, would it make sense as a global switch for other supports too? I think ideally they all should have a similar API.
Agreed, it'd be good to ensure whichever way we go, that it's consistent. As you mention, it could be quite handy for a theme to be able to disable a whole set of controls such as color. Also, it'd mean they could do that without worrying that in a subsequent release there might be additional controls added, which is often the case with typography, which keeps getting more features added.
I quite like enabled as a subkey for each block support, since it makes it explicit what's being set to false, however this is also a good point from Ramon:
A flag like enabled appears good: what does "enabled" mean though? Does it only relate to controls while keeping layout support (wide/content sizes) via theme.json?
If we like the idea of enabled, would it be worth implementing it across the board as a proposed shape for the API. That way, if it's documented in a consistent way for block supports, maybe that might help alleviate some potential confusion?
There was a problem hiding this comment.
If we went with it for layout, maybe allowEditing could be added to other support types too?
Oh, I like it too! It's more specific than enabled
There was a problem hiding this comment.
Sure, but there isn't a one-liner that removes a support altogether for a given block.
Ah, I see. I misunderstood. The point of using layout: false was intentional (not because you were unable to use a subkey). I like that approach, it's simple. However, it's problematic for supports that define presets. If we expand the same idea to color or typography we have:
{
"settings": {
"color": false,
"typography": false
}
}And then, how would we define the color.palette or typography.fontSizes if we depend on the top-level key to be false to disable "all the features" of that block support?
There was a problem hiding this comment.
Now that I understood the problem space you are exploring 😅 I remembered that there's an issue/PR somewhere that discussed the introduction of showInUI (perhaps a better name than enabled, though I personally like allowEditing more). The idea was similar to this: the features work, but are not shown in the UI.
We discussed making it a boolean, but also the ability to enable/disable features per editor, as in:
{
"settings": {
"layout": {
"showInUI": [ "siteEditor" ]
},
"color": {
"showInUI": [ "postEditor", "siteEditor" ]
}
}
}I'm not sure how much appetite there is for this now that most of the tools can be locked down already (not via single switch, though). Back then, the rationale that supported introducing such a flag was that agencies/admins/etc. could be interested in doing it for an individual post while maintaining them enabled site-wide. Though a boolean to lock them in all would still be useful, so it doesn't hurt starting that way. Making the flag "future-proof" (meaning it'll affect all new features introduced in the future) sounds appealing as well.
There was a problem hiding this comment.
Ooh interesting, I hadn't seen that work. Given that we seem to be moving in the direction of everything being editable through the site editor, it makes sense to start with a boolean; we can always revisit later.
|
Size Change: +322 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Cool PR! This is testing pretty well for me, for switching off controls for individual blocks, and I like how simple the approach is, too 👍
While testing this out, it raised a couple of questions / thoughts for me, when it comes to exposing the root of the layout object (layout) as being able to be set to false:
- For other block supports, such as spacing, folks set individual properties to
falserather than the whole thing. If they want to disable all controls, they'd do it one-by-one, by settingspacing.margin,spacing.padding, andspacing.blockGaptofalse. Would it be worth trying a similar granular approach to layout here? I was mostly thinking of this because: - Setting the
groupblock tolayout: falsemostly works, as folks can still transform between Row/Stack/Group, but I could imagine folks reaching for this setting primarily because they want to remove the custom content/wide size input fields, but still allow Row and Stack to have orientation controls. - Setting
layouttofalseat the root oftheme.jsonswitches off thecontentSizeandwideSizevalues, so it isn't possible to switch off layout controls altogether across blocks without affecting the site's styling in the way that you can switch offpaddingormarginacross a site.
Overall, I like the idea of the simplicity of setting a particular block to switch off the layout controls, though. And nothing about exposing false here precludes exploring individual feature-based false options in follow-ups 👍
I think my only real concerns are whether the behaviour at the root of theme.json is expected (settings.layout being set to false), and the question of changing the value in VALID_SETTINGS and whether that's safe. Otherwise, I think this is looking good!
Oh, one last thing: looks like we'll want to update the schema to reflect the new allowed values, too:
|
Great thoughts @andrewserong !
This might be solved once the layout controls are redesigned, as those inputs will lose prominence.
I'm feeling increasingly uncomfortable with how global contentSize and wideSize are coupled with layout 😅 might be good to explore some other options here. |
|
Thanks for the feedback everyone! I've updated to use "allowEditing" to switch off controls. |
There was a problem hiding this comment.
Sounds like we all like the allowEditing prop as a name, and the idea of starting with using it as a boolean 👍
This is all testing well for me:
✅ Can set to false at the block level in theme.json
✅ Can set to false at the root layout level in theme.json without affecting contentSize and wideSize values
✅ Block level in theme.json correctly overrides root layout level in theme.json
✅ Block level in theme.json cannot override false value set in a block's block.json (i.e. you can't turn editing on for the Columns block)
All LGTM! ✨
Shall we also update the theme.json schema for this new prop, or leave it for another PR?
gutenberg/schemas/json/theme.json
Line 280 in 02566bc
| 'wideSize' => null, | ||
| 'contentSize' => null, | ||
| 'wideSize' => null, | ||
| 'allowEditing' => null, |
There was a problem hiding this comment.
Doesn't need to happen in this PR, but should we add a @since 6.4.0 line in the doc comment above this const?
|
Should we also update the theme.json schema? I'm looking at "settingsPropertiesLayout", which I think is shared by "settings.blocks[blockName].layout". |
tellthemachines
requested review from
ajlende,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
Dev noteDisable layout controls from
|
|
I'm not able to get this new functionality to work. I have WP 6.4 RC-1 with Gutenberg 16.8.1. I've created a child theme of Twenty Twenty-Four and here is the I add a Group block in a new Post/Page and I can choose from: None, Wide Width and Full Width, which indicated to me this behavior is not functioning correctly in a child theme. I've also tried activating Twenty Twenty-Four theme (non-child) and placing the exact same code (above) within and I still have options for None, Wide Align and Full Width. I suspect there is key information missing from the Dev Notes in giving context to how this feature is expected to behave, which will likely cause users to misinterpret or feel failed when attempting to leverage. |
|
@colorful-tones The layout options this is intended to remove are these here in the Sidebar:
Not the alignment options |
|
Awesome, thanks @fabiankaegy . I hope that additional clarification makes it into the final Dev Note, because it has potential to cause confusion. |
|
Even just including your wonderful screenshot could help. 👍 |
What?
Fixes #47807.
Thinking better about this I'm not sure it should be possible to enable the controls from theme.json as even if the block supports layout already (say Columns or Gallery, which support layout but have controls hidden) it might not work well with the layout controls.
Testing Instructions
2.Add a Buttons block in the editor and check that no layout controls display in the sidebar.
Testing Instructions for Keyboard
Screenshots or screencast