Add setting to disable custom content size controls #56236
Conversation
tellthemachines
added
[Type] Enhancement
tellthemachines
requested review from
spacedmonkey,
ellatrix,
ajlende,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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 |
| 'contentSize' => null, | ||
| 'wideSize' => null, | ||
| 'allowEditing' => null, | ||
| 'allowCustomContentSize' => null, |
There was a problem hiding this comment.
I wonder if it might be worth adding some of the other settings that allow for disabling controls, such as allowOrientation or allowJustification. Currently these only work when set in block.json.
There was a problem hiding this comment.
Great idea! To keep things contained, I reckon we could explore those in separate PRs?
| const blockSupportAndThemeSettings = { | ||
| ...layoutSettings, | ||
| ...layoutBlockSupport, | ||
| }; |
There was a problem hiding this comment.
I'm undecided about the order of these. On one hand, it would make sense for it to be possible to override block default settings at a theme level. On the other, enabling layout controls on some blocks might result in a suboptimal experience (e.g. Gallery or Columns).
There was a problem hiding this comment.
My understanding was that we'd want theme.json to be able to disable something set in block.json (i.e. hide controls) but never to enable something that the block itself hasn't opted into.
There was a problem hiding this comment.
Then the order is correct, assuming controls aren't explicitly set to true in block.json. Which they shouldn't be, because all controls display by default.
|
Size Change: +24 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
| const { | ||
| allowSwitching, | ||
| allowEditing = allowEditingSetting ?? true, | ||
| allowEditing = true, | ||
| allowInheriting = true, | ||
| default: defaultBlockLayout, | ||
| } = layoutBlockSupport; | ||
| } = blockSupportAndThemeSettings; |
There was a problem hiding this comment.
It looks like this changes the logic slightly in that layoutBlockSupport now always overrides layoutSettings and so setting allowEditing in theme.json will no longer override the block.json setting?
Is it possible to do something like:
- Grab the block level (block.json) settings
- Iterate over the desired
theme.jsonsettings and if they're set to false, apply them over theblock.jsonsettings
That way if allowCustomContentSize is explicitly set to true in block.json but theme.json sets it to false, the latter wins. But if they're set the other way around block.json wins (essentially, false always wins).
Just a quick thought, though, happy to give this PR a more thorough review next week! 🙂
There was a problem hiding this comment.
Yeah that's what I was referring to in my previous comment! In principle, none of those settings should be explicitly enabled as their defaults are always true, and this will work as it is for all core blocks. Of course there's no way to control what third party block providers do with their settings, so e.g. a theme that disables controls at a global level won't be able to override a hypothetical custom block that explicitly enables settings. I don't see that as a huge issue though.
There's also the slight problem of contentSize and wideSize being set to false for all blocks in the settings. Though it's unlikely that a block would set them explicitly in its block.json, that's not something we'd want to override if it were set.
There was a problem hiding this comment.
Thanks for clarifying!
There was a problem hiding this comment.
I was a little confused by the false value for contentSize and wideSize, and I think I might have gone down a rabbithole 😄
From looking where it's set (here), I think I see why that's happening as a default now. It seems the assumption in useSettingsForBlockElement might have been that contentSize and wideSize would work similarly to other settings in that they'd be explicitly opted into. So they default to false in that hook, but a block might set either of these to true in block.json.
I was going to say, what if we used contentSize and wideSize as the boolean flag to show / hide the controls instead of allowCustomContentSize, but I'm not sure that would work because we already use these props at the root level to define the value of these fields instead of their visibility status. It's a bit unfortunate, because looking at it now, I wonder if it'd have made sense to have contentSize and wideSize as values under styles and to keep the boolean status in settings so that it'd match how padding/margin works?
At any rate, keeping a separate allowCustomContentSize prop (or allowContentSize, whichever naming works) sounds like the pragmatic path forward to me.
In terms of the issue of expanding values and passing them down via the layoutBlockSupport prop on layoutType.inspectorControls, would it be safer to merge just the values that we want to grab from settings for now? So similar to how it previously did allowEditing = allowEditingSetting ?? true, we might construct it something like:
const blockSupportAndThemeSettings = {
...layoutBlockSupport,
allowEditing: layoutSettings.allowEditing ?? true,
allowCustomContentSize: layoutSettings.allowCustomContentSize ?? true,
};
I haven't tried that out, so just an idea! And apologies if I'm overthinking it, I got quite lost looking up those default false values 😅
There was a problem hiding this comment.
That could work! We'll potentially have to do the same for all the other allow... settings. I guess there's not that many of them so it's not a huge issue.
There was a problem hiding this comment.
If I understand correctly, that's essentially what my previous code was doing; might as well revert the commit.
It's pretty close, but from re-testing, two things that aren't covered in the previous state are:
- If a value is explictly set to
trueinblock.json, it can't be overridden bytheme.json - If we wish to exclude other values in
layoutSettingsthat we don't want to pass down (like thefalsevalues forcontentSizeandwideSize)
Would it be better for consistency with other blocks supports to allow theme.json to override true values, where for example if spacing.padding is set to true in block.json, then it can still be overridden by theme.json?
I'd like to avoid iterating over specific settings if possible.
How come? I was thinking it could be good to be specific about the settings so that we know which ones are handled, otherwise it might be unexpected if or when someone goes to add additional properties that might expect different behaviour 🤔
There was a problem hiding this comment.
I'd like to avoid the iteration in order to keep the logic clear and ensure existing settings such as allowJustification or allowOrientation can be enabled just by adding them to VALID_SETTINGS (which I was planning to do in a follow-up PR)
Also, ideally the order of things should be predictable and not vary per setting; in this case, it seems best for block.json to override theme.json settings so that blocks that disable controls aren't overridden. In my mind it would be unexpected if different orders were applied to different settings.
If a value is explictly set to true in block.json, it can't be overridden by theme.json
Core blocks never explicitly set these values to true, and I wouldn't expect most custom blocks to do so either (this can be clarified in documentation to help extenders make good decisions, but regardless, if custom block authors really want controls to always be available then I'd say that's up to them, because they always have the option to create their own controls if core ones don't do what they want)
If we wish to exclude other values in layoutSettings that we don't want to pass down (like the false values for contentSize and wideSize)
contentSize and wideSize ending up in this layout settings object shouldn't affect anything as we're not using them at all, unless I'm missing something! The only place this object is being used is in the layout inspector controls.
There was a problem hiding this comment.
Okay, great, thanks for thinking this through with me! That all sounds reasonable to me, especially once we document it, and it certainly is more readable the way that you've done it 👍
I suppose the last question then is that pesky naming question of what we call the setting 😄
There was a problem hiding this comment.
😅
I honestly don't have any better ideas, especially given the other layout controls all follow the same pattern. And Lightbox support is also using allowEditing as of 6.4.
There was a problem hiding this comment.
Me either. I don't feel strongly about the naming, but I'd personally lean toward either allowCustomContentSize or allowContentSize since we have that prefix for the other layout controls, as you mention, and we likely want parity between what block.json uses and what theme.json uses. The thing that I like about allow as a prefix is that it's clearer it's about what's exposed in the UI, rather than what's output. I think the layout support is possibly a bit unique in this respect, where it can be helpful to have that extra clarity.
Also, the word custom in allowCustomContentSize is good, because we're still allowing a user to switch between flow and constrained layout types, where content size is output via the default layout styles... so of all the available options so far, I think allowCustomContentSize is my favourite.
|
Can we remove "allow" from allowCustomContentSize? The other settings use these names: customDuotone |
@carolinan all the layout properties use "allow". The difference from the other settings you mention is that these layout settings don't disable the functionality itself, but only the user-facing controls. So a block with |
|
I don't think that intention is clear. Not by the name, or by how it works. If I add "customFontSize":false, but add this to the markup, the block uses the value I set correctly, without block validation errors or similar: |
There was a problem hiding this comment.
This is testing quite well for me, thanks again for the discussion here:
✅ Setting allowCustomContentSize at the root layout object allows globally hiding the controls
✅ Setting the allowCustomContentSize value at the block level in theme.json allows you to override the global value (so you can show everywhere except Group block, or hide everywhere except Cover block, for example)
I was curious about the naming, too — I don't have a strong opinion about it, but I wondered briefly if we could just use contentSize and wideSize instead of an additional prop prefixed via allow. Unfortunately, because of how we set content/wideSize to a pixel value in the root layout object, I don't think that's possible, but I left a comment with where I got to in investigating.
Whichever naming convention we go with, my main opinion is that we pick something we're happy sticking with, whichever name that might be 🙂
| const { | ||
| allowSwitching, | ||
| allowEditing = allowEditingSetting ?? true, | ||
| allowEditing = true, | ||
| allowInheriting = true, | ||
| default: defaultBlockLayout, | ||
| } = layoutBlockSupport; | ||
| } = blockSupportAndThemeSettings; |
There was a problem hiding this comment.
I was a little confused by the false value for contentSize and wideSize, and I think I might have gone down a rabbithole 😄
From looking where it's set (here), I think I see why that's happening as a default now. It seems the assumption in useSettingsForBlockElement might have been that contentSize and wideSize would work similarly to other settings in that they'd be explicitly opted into. So they default to false in that hook, but a block might set either of these to true in block.json.
I was going to say, what if we used contentSize and wideSize as the boolean flag to show / hide the controls instead of allowCustomContentSize, but I'm not sure that would work because we already use these props at the root level to define the value of these fields instead of their visibility status. It's a bit unfortunate, because looking at it now, I wonder if it'd have made sense to have contentSize and wideSize as values under styles and to keep the boolean status in settings so that it'd match how padding/margin works?
At any rate, keeping a separate allowCustomContentSize prop (or allowContentSize, whichever naming works) sounds like the pragmatic path forward to me.
In terms of the issue of expanding values and passing them down via the layoutBlockSupport prop on layoutType.inspectorControls, would it be safer to merge just the values that we want to grab from settings for now? So similar to how it previously did allowEditing = allowEditingSetting ?? true, we might construct it something like:
const blockSupportAndThemeSettings = {
...layoutBlockSupport,
allowEditing: layoutSettings.allowEditing ?? true,
allowCustomContentSize: layoutSettings.allowCustomContentSize ?? true,
};
I haven't tried that out, so just an idea! And apologies if I'm overthinking it, I got quite lost looking up those default false values 😅
The earliest settings with that name were added in #34194, so I think that ship has sailed. We could rename them, but we'll still have to provide support for the existing ones indefinitely. Also, naming things clearly is hard. What could we call these settings alternatively?
Oh I hadn't realised you could do that. Still, you wouldn't be able to set a custom font size in theme.json and disable the control for a particular block, whereas with the existing layout settings we can e.g. set justification for a block and also disable the justification controls. I'm not sure how widely used that might be, but it's how all the layout controls work so far (the only ones that don't have the |
|
Flaky tests detected in 53061ef. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6938635943
|
That's a good question, I'm not sure either. There doesn't seem to be a 1:1 match between properties on those two arrays - the JS one has |
There was a problem hiding this comment.
The shape of this PR is looking good to me, and the behaviour is working correctly for me as we discussed in the comments 👍
It seems like the final question is about the naming of the property. I personally quite like allowCustomContentSize as it follows the structure of the other boolean layout properties, but it might be worth getting a second opinion as it could be difficult to change the name after the fact if we want to, and since @carolinan had some concerns about the clarity of the name.
I'll just ping @WordPress/gutenberg-core in case anyone has strong opinions about it. I seem to recall having discussions with @oandregal about naming of theme.json properties in the past 🙂
+1 The flag determines UI control visibility, and doesn't described a specific value or property so I think it's generic enough to handle iterations to the control itself. Also Just my 1.99 cents |
|
One thing I have been wondering about controls like this - do we want them at a theme level or a site level? The way I see it, the job of theme.json is to define the visual appearance of a site. I wonder if settings like this are more aimed at situations where a developer wants to lock down access to certain features on a site. If that is the case maybe it makes more sense for these settings to live in a different configuration (maybe site.json). This would have the advantage of staying in place even if a user switches theme, and allowing theme.json to be used solely for the purpose of controlling a site's visual appearance. |
I also prefer the name In other words, I'm considering the following possibilities in the future.
|
Naming, right? I reckon I have my weaknesses 😆 Quickly glanced through the conversation and the existing theme.json settings. I see layout already uses Another point is that it seems That's more or less how would I think about this topic. Though whatever you feel right is good 🙂 |
|
Good discussion, folks!
That's a good question — I suppose one way to look at it is that it should be possible for a theme to lock down the visual controls and also for a site owner to be able to do so. The former case is about which controls are exposed in the UI, so while UI controls are hidden, it's not really defining access controls in that block markup with particular values can still be used. The latter case I could imagine being a fair bit more complex: if we're trying to prevent users from being able to make certain kinds of changes, then folks might want stricter protections on preventing block markup with certain features, so we might also need another level of validation on block markup?
Folks are already using |
|
Thanks for the feedback folks!
I'm not convinced there'll ever be a use case for keeping either content or wide size editable without the other. We could perhaps go with |
If the developer is the theme author, it makes sense for these settings to continue in theme.json. I'm considering the ability to change certain visual features as part of the definition of the visual appearance of the site. If we think of theme.json as a sort of style guide, then ability to edit settings does have a place in there. Locking down features at a site level would give control of these choices to site admins instead of developers, which I'm unsure is what we want here. It might be worth researching some use cases here - I think agencies would be able to give us some good feedback on what's most useful.
Correct; disabling layout controls overall is already available in theme.json since WP 6.4 so removing support is no longer an option. Given the global setting exists, it also makes sense to provide individual settings. |
tellthemachines
force-pushed
the
add/allow-content-size
branch
from
e12a91a
to
511372f
Compare
|
Ok; updated the setting name to |
Looking good! Apologies for one last naming suggestion, but just looking over this again, and wondered if |
Yeah sorry I didn't explain my whole reasoning above. |
There was a problem hiding this comment.
Thanks for all the back and forth here and for the explanation! This is all testing well for me, and I think we've settled on a good name factoring in all the trade-offs 👍
✅ In theme.json, can set layout.allowCustomContentAndWideSize to false to disable controls across all blocks
✅ In theme.json, can set layout.allowCustomContentAndWideSize to true to enable controls for a single block as an exception
✅ Can disable at the block level in block.json by setting it to false
I've just updated the example in the PR description to reflect the new name for the prop.
LGTM! ✨
The reason for my hesitance here is that the way I see it, theme.json files should only be concerned with the visual appearance of a site, not other configuration settings. I know that we already have other settings in theme.json that don't fit with this model (for example templates) but I just wanted to share where I think we should be moving towards. That shouldn't block this work, but just wanted to raise it as something to keep in mind for the future :) |
Isn't the configurability of the visual appearance a visual appearance concern though? Or do you reckon that theme.json should only contain settings that translate directly to CSS? I'm not sure to what extent changing the current approach of styles + settings will be possible given we'll have to continue supporting it forever. Conceptually, I'm not super invested either way 😄 but I do think it's important to be clear on who should be responsible for the decision to expose these controls or not. To me it makes sense that any controls related to appearance should be defined by the theme author as part of the theme, so if it's not in theme.json it should be in another theme file. An argument could also be made for site admins to own these settings, or for both: site config could exist on top of theme config. |
* Add setting to disable custom content size controls * Make sure theme setting overrides block setting. * Revert "Make sure theme setting overrides block setting." This reverts commit ad4b76c. * Change setting name to `allowCustomContentAndWideSize`
|
Dev note: Setting to disable custom content size controlsSimilar to the setting that allows disabling layout controls in WP 6.4, this allows only the content and wide size controls for constrained layout to be disabled from theme.json, globally or at a per-block level . To disable for all blocks globally, add the following in theme.json under
To disable at the block level, add the following in theme.json under |
tellthemachines
added
the
has dev note
What?
Similar to the setting that allows disabling layout controls in #53378, this allows only the content and wide size controls for constrained layout to be disabled from theme.json, globally or at a per-block level (note that disabling globally only affects the controls displayed in the block sidebar, not the ones under global styles > layout)
Testing Instructions