Add layout content size controls to global styles #42309
Conversation
|
Size Change: +702 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Oh, this is testing quite nicely so far! I was wondering at first if the HStack component might help simplify things a bit versus the Flex component (it doesn't seem to need the children the be FlexItems?), but I don't think it helps directly with the icon positioning. Shame that UnitControl's positioning doesn't appear to neatly allow additional elements at the same vertical position as the input field 🤔
The UI here also demonstrates nicely a point you made in the root CSS PR that these controls seem to fit logically with padding and other dimensions controls, they look good alongside each other, all under a Layout panel. Not sure what that means for future iterations of the block level inspector controls, but at least from my perspective, these controls look nicely at home in this panel! 👍
Not really related to this PR, but I noticed while testing that the Reset to Defaults button in the dropdown correctly resets the values, but not the ToolsPanel state — so the ellipsis menu thinks that the value is still customised. It doesn't appear to actually cause any issues so probably not something to worry about in this PR, but just thought I'd mention it:
|
Thanks for testing @andrewserong ! I hadn't thought of As a side note, we can use the Another option would be to add styles directly instead of using these components, and that would allow us to make micro-adjustments to margins as needed.
Ah, well spotted! Testing on trunk, it seems like the global reset doesn't change the padding value in the input either - it does reset it, but the number only disappears after save and refresh. Might be best to look into that behaviour separately. |
|
Update: opened #42380 for FlexItem swallowing its children. |
tellthemachines
force-pushed
the
add/content-size-in-global-styles
branch
from
8b73485
to
bd6e4af
Compare
tellthemachines
requested review from
scruffian,
MaggieCabrera,
ramonjd and
aaronrobertshaw
tellthemachines
added
Global Styles
There was a problem hiding this comment.
The positioning in the UI's looking good now @tellthemachines! Do we need to add the explanatory text from the editor UI for the content/wide sizes, too?
Updating the content and wide size at the root level is working well, but the reset behaviour is still a bit unreliable for me. Also, I found an edge case with blocks that don't display content/wide size, and added a comment about the weird ToolsPanel menu behaviour — unfortunately I couldn't quite work out what's going on there 🤔
Another thought is that the resetAll function passed to the ToolsPanel should probably include the reset calls for the content and wide sizes, too... but probably only if the showContentSizeControl and showWideSizeControl variables are truthy?
Happy to do more testing!
| @@ -141,6 +178,66 @@ export default function DimensionsPanel( { name } ) { | |||
|
|
|||
| return ( | |||
| <ToolsPanel label={ __( 'Dimensions' ) } resetAll={ resetAll }> | |||
| <Flex> | |||
There was a problem hiding this comment.
It looks like this row might need to be behind a check for showContentSizeControl || showSideSizeControl, otherwise in the Layout panel at the block level, there is a larger space due to the gap surrounding the empty Flex component:
There was a problem hiding this comment.
Oh, yes, definitely!
| name | ||
| ); | ||
|
|
||
| const hasContentSizeValue = () => !! contentSizeValue; |
There was a problem hiding this comment.
Taking another look at this check, I'm wondering if part of the issue with the ToolsPanel menu state is that if the contentSizeValue matches the value after reset, then this will return true. So if my content width is set to 840px and I change it to 900px and then reset it, the value here is 840px, and so the menu item thinks there's still a custom value?
That said, there seems to be something else funny going on, because when I first load the site editor, the ToolsPanel seems to be showing padding as customised, too 🤔
The weird behaviour does appear to be on trunk, too, so not sure if it's worth attempting to fix now? I think on trunk it's a little less visible because it seems to be working fine at the block level, and we currently only have the Padding dimensions control made visible at the root level 🤔
There was a problem hiding this comment.
So if my content width is set to 840px and I change it to 900px and then reset it, the value here is 840px, and so the menu item thinks there's still a custom value?
It appears that the menu is reflecting exactly what the hasContentSizeValue callback is telling it. Any truthy value for contentSizeValue is going to state that the ToolsPanelItem has a customized value.
If I understand the situation correctly, there's always a root value so the logic here needs to be more nuanced. It should be checking if the user has set a value i.e. check the style value from the user origin rather than the merged styles aka all origin.
The border panel checks the user origin border styles to reflect the whether the user has customized any values e.g. const [ userBorderStyles ] = useStyle( 'border', name, 'user' );
It would be a worthwhile follow up to give these global styles panels an audit to check they all behave appropriately. I know things have evolved rapidly here on multiple fronts so its entirely possible a few gremlins crept in.
There was a problem hiding this comment.
Checking for user styles makes sense! Updated.
There was a problem hiding this comment.
Thanks for your work on this @tellthemachines 👍
In terms of the content/width size, it is testing pretty well for me. I hit similar issues as @andrewserong however with the new controls/panel behaviour.
Before digging too deep into things, it might be worth highlighting here a recent issue found that impacts resetting input control based components for which @stokesman already has a fix up for in #42484.
Cherry picking the fix from that PR fixed a couple of resetting issues encountered while testing this one.
Not really related to this PR, but I noticed while testing that the Reset to Defaults button in the dropdown correctly resets the values, but not the ToolsPanel state
Global Styles' option to "reset to defaults" clears all the values independently. Given it happens outside the ToolsPanel callbacks there is nothing to trigger its internal state update. (Would it be too hacky to force a new ToolsPanel when the global styles are reset?)
In some past discussions it was raised that clearing a value from the field could be a customization and therefore we couldn't really catch the fact the field was empty and automatically remove the customized status reflected in the panel menu. The thinking at the time was that this wasn't a real issue. I'm not sure if that needs revisiting though via a follow-up.
Long story, short. I don't think the "reset to defaults" issue should block this PR at all.
I've also left a couple of other inline comments that might help smooth out the customized value display or reduce some markup. Hope that helps.
| <FlexItem> | ||
| <ToolsPanelItem | ||
| label={ __( 'Content size' ) } | ||
| hasValue={ hasContentSizeValue } | ||
| onDeselect={ resetContentSizeValue } | ||
| isShownByDefault={ true } | ||
| > |
There was a problem hiding this comment.
ToolsPanelItem is polymorphic as well so you should be able to just add the as={ FlexItem } to it right?
There was a problem hiding this comment.
Ahhhh thanks hadn't thought of that!
| name | ||
| ); | ||
|
|
||
| const hasContentSizeValue = () => !! contentSizeValue; |
There was a problem hiding this comment.
So if my content width is set to 840px and I change it to 900px and then reset it, the value here is 840px, and so the menu item thinks there's still a custom value?
It appears that the menu is reflecting exactly what the hasContentSizeValue callback is telling it. Any truthy value for contentSizeValue is going to state that the ToolsPanelItem has a customized value.
If I understand the situation correctly, there's always a root value so the logic here needs to be more nuanced. It should be checking if the user has set a value i.e. check the style value from the user origin rather than the merged styles aka all origin.
The border panel checks the user origin border styles to reflect the whether the user has customized any values e.g. const [ userBorderStyles ] = useStyle( 'border', name, 'user' );
It would be a worthwhile follow up to give these global styles panels an audit to check they all behave appropriately. I know things have evolved rapidly here on multiple fronts so its entirely possible a few gremlins crept in.
|
Thanks for the reviews, folks!
I think the text we see currently in the block Layout panel ("Customize the width for all elements that are assigned to the center or wide columns.") is a bit confusing even in the block context, and more so here because we're setting widths site-wide here, so "customize" in this case just means "set a new global value". Is it just me? Perhaps we should rethink that copy, and think of a specific one for this global control. Would something like "Set the width of the main content area" work here?
Was that the Flex markup showing, or did I miss something else?
Ah, good to know! If that one gets merged soon I'll rebase this PR.
I'm not sure. Thinking about the UX, it's unlikely someone will go into the individual reset panels after resetting all, unless the global reset didn't work? Maybe we can look at that as a separate task and consider the options. One thing I'm not sure about: currently, these controls only display if the theme has contentSize/wideSize set in its |
My understanding is that one of the features of theme.json is that it allows theme authors to lock down what is available in the Global Styles UI. For example, they could lock down editing color palettes, prevent users from applying custom borders etc. If a theme has not opted to set |
Not just you! I find it confusing, too. If it's too awkward to do here, perhaps tweaking how and when we display explanatory information for this feature is best explored in a follow-up / separate PR at some stage? But I quite like "Set the width of the main content area" personally 👍
Nope, that was all!
Oh, that's an interest idea. I don't think I'd even really thought of locking down this part of the UI (I'd mostly been thinking about when themes want to lock down the post editor, rather than global styles). Perhaps a boolean in theme settings that defaults to |
Hmmm, in that case it would make sense for themes be able to both set content sizes and stop users from overriding them. If so, and if
this should probably also apply to the individual block controls, right? And if we had something like this, we needn't assume that a theme without content sizes doesn't want users to be able to set them at all. |
|
Update on the description: I tried to add a paragraph inside the
I've created #42543 to report what seems to be a I guess we can add the description as a separate PR once the issue is fixed? I'm thinking the bit about allowing themes to explicitly control whether content and wide sizes are editable can also be tackled separately, as it'll probably generate a bit of discussion 😅 If that's the case, I think all other feedback on this PR has been addressed! |
There was a problem hiding this comment.
If that's the case, I think all other feedback on this PR has been addressed!
Yes, this PR is working nicely for me now, and I agree I think adding messaging and figuring out the appropriate behaviour for showing/hiding the controls are good candidates for follow-up PRs.
✅ Adjusting content and wide width is now treated in the ToolsPanel menu as a changed value if a user set value exists
✅ Clearing each value in the menu individually works correctly
✅ Clearing via the Reset all option works correctly for these two values
✅ Styling and behaviour of the Dimensions panel at the block level in global styles is as on trunk
LGTM, thanks for all the follow-up tweaks! 🎉
|
I think we might be overcomplicating the addition of these controls to the panel. 🤔 Am I correct in understanding that the wrapping The The same way the Finally, for the help text, this is a bit of an edge case. Normally, help text would relate to a single control and be included as a child within the In the example diff below I added a simple Click for example diffdiff --git a/packages/edit-site/src/components/global-styles/dimensions-panel.js b/packages/edit-site/src/components/global-styles/dimensions-panel.js
index a2f8822d66..e7af30623c 100644
--- a/packages/edit-site/src/components/global-styles/dimensions-panel.js
+++ b/packages/edit-site/src/components/global-styles/dimensions-panel.js
@@ -6,10 +6,10 @@ import {
__experimentalToolsPanel as ToolsPanel,
__experimentalToolsPanelItem as ToolsPanelItem,
__experimentalBoxControl as BoxControl,
+ __experimentalHStack as HStack,
__experimentalUnitControl as UnitControl,
__experimentalUseCustomUnits as useCustomUnits,
- Flex,
- FlexItem,
+ __experimentalView as View,
} from '@wordpress/components';
import { __experimentalUseCustomSides as useCustomSides } from '@wordpress/block-editor';
import { Icon, positionCenter, stretchWide } from '@wordpress/icons';
@@ -193,65 +193,59 @@ export default function DimensionsPanel( { name } ) {
return (
<ToolsPanel label={ __( 'Dimensions' ) } resetAll={ resetAll }>
{ ( showContentSizeControl || showWideSizeControl ) && (
- <Flex>
- { showContentSizeControl && (
- <ToolsPanelItem
- as={ FlexItem }
- label={ __( 'Content size' ) }
- hasValue={ hasUserSetContentSizeValue }
- onDeselect={ resetContentSizeValue }
- isShownByDefault={ true }
- >
- <Flex align="flex-end">
- <FlexItem>
- <UnitControl
- label={ __( 'Content' ) }
- labelPosition="top"
- __unstableInputWidth="80px"
- value={ contentSizeValue || '' }
- onChange={ ( nextContentSize ) => {
- setContentSizeValue(
- nextContentSize
- );
- } }
- units={ units }
- />
- </FlexItem>
- <FlexItem>
- <Icon icon={ positionCenter } />
- </FlexItem>
- </Flex>
- </ToolsPanelItem>
- ) }
-
- { showWideSizeControl && (
- <ToolsPanelItem
- as={ FlexItem }
- label={ __( 'Wide size' ) }
- hasValue={ hasUserSetWideSizeValue }
- onDeselect={ resetWideSizeValue }
- isShownByDefault={ true }
- >
- <Flex align="flex-end">
- <FlexItem>
- <UnitControl
- label={ __( 'Wide' ) }
- labelPosition="top"
- __unstableInputWidth="80px"
- value={ wideSizeValue || '' }
- onChange={ ( nextWideSize ) => {
- setWideSizeValue( nextWideSize );
- } }
- units={ units }
- />
- </FlexItem>
- <FlexItem>
- <Icon icon={ stretchWide } />
- </FlexItem>
- </Flex>
- </ToolsPanelItem>
- ) }
- </Flex>
+ <span className="span-columns">
+ Set the width of the main content area.
+ </span>
+ ) }
+ { showContentSizeControl && (
+ <ToolsPanelItem
+ className="single-column"
+ label={ __( 'Content size' ) }
+ hasValue={ hasUserSetContentSizeValue }
+ onDeselect={ resetContentSizeValue }
+ isShownByDefault={ true }
+ >
+ <HStack alignment="flex-end">
+ <UnitControl
+ label={ __( 'Content' ) }
+ labelPosition="top"
+ __unstableInputWidth="80px"
+ value={ contentSizeValue || '' }
+ onChange={ ( nextContentSize ) => {
+ setContentSizeValue( nextContentSize );
+ } }
+ units={ units }
+ />
+ <View>
+ <Icon icon={ positionCenter } />
+ </View>
+ </HStack>
+ </ToolsPanelItem>
+ ) }
+ { showWideSizeControl && (
+ <ToolsPanelItem
+ className="single-column"
+ label={ __( 'Wide size' ) }
+ hasValue={ hasUserSetWideSizeValue }
+ onDeselect={ resetWideSizeValue }
+ isShownByDefault={ true }
+ >
+ <HStack alignment="flex-end">
+ <UnitControl
+ label={ __( 'Wide' ) }
+ labelPosition="top"
+ __unstableInputWidth="80px"
+ value={ wideSizeValue || '' }
+ onChange={ ( nextWideSize ) => {
+ setWideSizeValue( nextWideSize );
+ } }
+ units={ units }
+ />
+ <View>
+ <Icon icon={ stretchWide } />
+ </View>
+ </HStack>
+ </ToolsPanelItem>
) }
{ showPaddingControl && (
<ToolsPanelItem
diff --git a/packages/edit-site/src/components/sidebar/style.scss b/packages/edit-site/src/components/sidebar/style.scss
index b3992b7774..41960d3ee0 100644
--- a/packages/edit-site/src/components/sidebar/style.scss
+++ b/packages/edit-site/src/components/sidebar/style.scss
@@ -60,6 +60,10 @@
grid-column: span 1;
}
+.edit-site-global-styles-sidebar .components-tools-panel .span-columns {
+ grid-column: 1 / -1;
+}
+
.edit-site-global-styles-sidebar__blocks-group {
padding-top: $grid-unit-30;
border-top: $border-width solid $gray-200;
After applying the above diff, this is what I get:
One thing to note is that the panel's CSS grid means spacing between the "Set the width of the main content area." text and the related controls. That might not be desired so please take the suggested diff with a grain of salt. If nothing else, I hope that helps illustrate a different approach to adding controls via the |
Oooh I see that works well! Would be good to add some tips along those lines to the |
Done 👍
I'll make that happen 🙂 |
packages/edit-site/src/components/global-styles/dimensions-panel.js
Outdated
Show resolved
Hide resolved
|
Dropping by for a quick look. Some options are natural to add inside the 3 dot menu. Other options should in general always be visible. |
andrewserong
added
the
[Feature] Layout
What?
Adds content size and wide size controls to global styles, under "Layout"
Why?
Fixes #35955.
How?
contentSizeandwideSizeinROOT_BLOCK_SUPPORTS;Testing Instructions
Screenshots or screencast