Show child layout controls by default #49389
Conversation
tellthemachines
requested review from
SaxonF,
jasmussen,
andrewserong and
apeatling
|
Size Change: +1.09 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Thanks for keeping this separate! Just to be sure, this shows the width control for row and stack children, but not those of group, right? I'm leaning towards this being a good idea, but would love additional thoughts. |
| @@ -612,7 +613,11 @@ export default function DimensionsPanel( { | |||
| hasValue={ hasChildLayoutValue } | |||
| label={ childLayoutOrientationLabel } | |||
| onDeselect={ resetChildLayoutValue } | |||
| isShownByDefault={ defaultControls.childLayout } | |||
| isShownByDefault={ | |||
| typeof defaultControls.childLayout === 'boolean' | |||
There was a problem hiding this comment.
Why it's not always a boolean?
There was a problem hiding this comment.
If I'm understanding properly, the check is less about type "boolean" and more about checking if the value is actually set to avoid considering "undefined" as "false".
I think I'd prefer if we stay consistent between all the default controls. We're considering undefined as false for all the default controls in all the *Panel components. So my question is: can we just keep it as is here? and update the "block.json" files for the blocks that override the default defaultControls instead?
There was a problem hiding this comment.
If I'm understanding properly, the check is less about type "boolean" and more about checking if the value is actually set to avoid considering "undefined" as "false".
Yes, that's the point of this check.
We're considering undefined as false for all the default controls in all the *Panel components. So my question is: can we just keep it as is here? and update the "block.json" files for the blocks that override the default defaultControls instead?
That's exactly what this change addresses: we need to have a way of defaulting to true for the child layout controls, but still preserving the ability to override that if needed by setting it to false in block.json. Child layout controls can be applied to any block, including third party blocks, so it's not practical to explicitly set the controls to display for every single block type.
There was a problem hiding this comment.
I like the idea of exposing false as a valid value, and the idea that we can have some controls be opt-out (in terms of the default controls) rather than opt-in.
If consistency for each of the controls is important, what if we used something like isShownByDefault={ defaultControls.childLayout ?? FALLBACK_DEFAULT_CONTROLS.childLayout } for each of the controls. Then, FALLBACK_DEFAULT_CONTROLS could contain the global defaults we'd like to go with. So if all the others are hidden by default, it might be:
const FALLBACK_DEFAULT_CONTROLS = {
...
padding: false,
margin: false,
blockGap: false,
minHeight: false,
childLayout: true,
};
Would something like that work? The idea's basically the same as this PR, but just that we could use the same approach as this PR for each of the other controls, with a const to contain our desired defaults.
There was a problem hiding this comment.
isShownByDefault={ defaultControls.childLayout ?? FALLBACK_DEFAULT_CONTROLS.childLayout }
This works for me 👍
There was a problem hiding this comment.
Thanks for the suggestion! I'll give it a go. Note that doing that will also require us to explicitly set all the controls to true on the site editor side, given that currently the site editor isn't passing any defaultControls into the Dimensions panel, so it depends on the values from DEFAULT_CONTROLS to be true.
There was a problem hiding this comment.
Ok I've added the suggested changes!
The child width/height controls currently only show for Row, Stack and Navigation children. They could be enabled for other blocks with flex layout, but they don't work on non-flex layouts for now. |
|
Flaky tests detected in 11df198. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4571109602
|
There was a problem hiding this comment.
Thanks for the updates, this is testing well for me!
✅ Can override by setting supports.spacing.__experimentalDefaultControls.childLayout to false, or by setting supports.dimensions.__experimentalDefaultControls.childLayout to false (since spacing and dimensions get merged for the DimensionsPanel)
✅ true and false values work correctly for padding
✅ true and false values work correctly for margin
✅ true and false values work correctly for blockGap
✅ true and false values work correctly for childLayout
✅ global styles still displays everything (except for childLayout of course) by default, and the changes to contentSize and wideSize don't affect anything in the block editor since they're not used anywhere
It's also really nice that there's a list of defaults, one each for those used by the block editor and the site editor, as it'll be really quick to make subsequent changes to the defaults if need be 👍
LGTM! ✨
| padding: false, | ||
| margin: false, | ||
| blockGap: false, | ||
| minHeight: false, |
There was a problem hiding this comment.
I'm sorry I don't understand why we had to change the default values here? Yeah, there's a small potential of changing the UI for existing blocks, but for me "true" makes more sense as default values for all of these.
There was a problem hiding this comment.
In other words, a user of DimensionsControl shouldn't have to define the "defaultControls" prop in order to have a working component.
There was a problem hiding this comment.
If you're worried about the changes for blocks, we can just update the block.json of the affected blocks?
The other piece of feedback I have is that we should be consistent in behavior between all the *Panel component (we have 4 right now I believe)
There was a problem hiding this comment.
Any thoughts on this? I'm actually not really satisfied that this PR changed the behavior of default controls in one Styles UI component but not the others, I'm also not convinced that this is the right behavior. Global styles are not customizable by the user, so they shouldn't need to pass any "defaultControls" prop, while the "blocks" (block inspector) are the ones that can customize the default controls, so they should be the ones providing a custom value for the defaultControls prop.
There was a problem hiding this comment.
It's a good question @youknowriad. When reviewing this one, I quite liked the idea of the panel itself defining the defaults, and then the site editor/global styles as an exception specifying that it wants to display all the controls. In that way, the defaults at the component level follow the expected behaviour in the block editor. But I can see the goal in preferencing things one way or the other.
Either way, I agree that it'd be good to make sure it's consistent across the different panels. If we're to do that, do you have a preference for how to proceed while maintaining displaying the child layout controls by default?
I've also left a similar comment on the styles UI documentation PR (#49720 (comment)) so that these discussions are connected.
There was a problem hiding this comment.
The main reason I went with this option was to not change things for third party blocks. Previously, the default was none of the controls would display except for global styles, but the default setting for the controls was to display all. Blocks outside of global styles weren't displaying controls because the defaults were being silently overridden by an empty object, which is non-ideal. But that's how things have been for a while, and if we change the default behaviour now, any custom blocks using these block supports will suddenly have a bunch of controls displaying by default that they didn't have before.
Personally, I'd love to have everything display by default, because having to look for controls in dropdowns is excruciatingly painful UX, but I'm concerned about changing things around for extenders with no warning.
I'm happy to make the same change for the other block supports as long as we're clear this is the way we want to go.
There was a problem hiding this comment.
I agree that it's not idea to change behavior on block authors but maybe we can make some small adjustments as it's not entirely a breaking change, things still work, they may just be hidden or visible. I think a dev note in this case would be cool.
Maybe we should try to make the change with the least disruption to block authors while making sure the default values are decent and consistent across the components. what would that be?
There was a problem hiding this comment.
making sure the default values are decent and consistent across the components. what would that be?
I'd be happy to make them all visible by default all the time, but perhaps best defer to @WordPress/gutenberg-design 😄
ndiego
added
[Feature] Layout
What?
Follow up from this conversation. Child layout controls are currently hard to find, and they only display when a block is inside a flex layout block, so it makes sense for them to display by default in those circumstances.
Testing Instructions
block.json, try settinginside the spacing block support, refresh the page and check that the controls no longer display by default.
Testing Instructions for Keyboard
Screenshots or screencast