Components: Add opt-in prop for 40px default size for `BoxControl`, `BorderControl`, and `BorderBoxControl` #56185

brookewp · 2023-11-16T02:50:36Z

What?

Add a new opt-in prop __next40pxDefaultSize to BoxControl, BorderControl, and BorderBoxControl, following the plan outlined in above mentioned PR.

Why?

For more consistency in styling. I've grouped these three together since they are very similar.

How?

A new, temporary __next40pxDefaultSize prop. When the prop is set to true, the input will have a height of 40px.

`BoxControl`

I've included some changes to the width when the prop is enabled, otherwise, the text isn't visible. I used the sizing from BorderControl's compact size, but I'd like to hear from @WordPress/gutenberg-design on this.

Before	After

Also, due to the narrow size, only 2-3 numbers will be visible without a horizontal scroll.

Testing Instructions

In Storybook:

npm run storybook:dev
See initially that each component hasn't changed
Enable the prop / set to true
See the new size
Ensure all inputs are affected and match

In the editor

Smoke test the components in the editor; BoxControl, BorderControl, and BorderBoxControl shouldn't have any visible changes for now.

Note

BoxControl is only used in DimensionsPanel for backward compatibility. The only way I've been able to test it is by setting showSpacingPresetsControl to true here:

gutenberg/packages/block-editor/src/components/global-styles/dimensions-panel.js

Lines 487 to 499 in d69ee81

    
           { ! showSpacingPresetsControl && ( 
        
           	<BoxControl 
        
           		values={ paddingValues } 
        
           		onChange={ setPaddingValues } 
        
           		label={ __( 'Padding' ) } 
        
           		sides={ paddingSides } 
        
           		units={ units } 
        
           		allowReset={ false } 
        
           		splitOnAxis={ isAxialPadding } 
        
           		onMouseOver={ onMouseOverPadding } 
        
           		onMouseOut={ onMouseLeaveControls } 
        
           	/> 
        
           ) }

However, __next40pxDefaultSize causes issues in BoxControl, where the text in the input is not visible due to the width constraints of the inspector. So we may need to pause this for BoxControl to reassess the design.

jameskoster · 2023-11-16T11:32:00Z

For BorderBoxControl the borders are fine in Storybook, but the top border seems to be missing in the Inspector:

Correct me if I am wrong but I don't think BoxControl is used anywhere, so I couldn't smoke test that one.

Thanks for powering thought all of these!

ciampo

I see why using the __next40pxDefaultSize to force size to __unstable-large makes sense for this PR, since it keeps code changes to a minimum. We will be able to clean up this code after deprecating/removing both the __next40pxDefaultSize prop, and the __unstable-large value for the size prop (see logistics)

When both __next40pxDefaultSize and isCompact are true, BorderControl is not wide enough to show the border width:

Not related to this PR (since it can be observed on trunk too), but I realised the BorderControl's dropdown toggle size looks off:

This is probably due to some updates to other components that caused the height of the unit control and the toggle to go out of sync — the 30px width/height specified here should be updated:

the simplest update would be to change 30px to 32px
it would also be tempting to remove those hardcoded values altogether, and use the size and __next40pxDefaultSize props directly on Button

packages/components/src/border-control/border-control/component.tsx

packages/components/src/border-box-control/border-box-control/component.tsx

ciampo · 2023-11-17T14:48:27Z

packages/components/src/border-box-control/border-box-control/component.tsx

 					/>
 				) }
 				<BorderBoxControlLinkedButton
 					onClick={ toggleLinked }
 					isLinked={ isLinked }
-					size={ size }
+					size={ inputHeight }


Nit: we can rewrite the linked button styles in a way that doesn't need the size prop at all. I also realized that the linked button uses the isSmall deprecated prop, which should be replaced by size="small"

Something like this:

diff --git a/packages/components/src/border-box-control/border-box-control-linked-button/component.tsx b/packages/components/src/border-box-control/border-box-control-linked-button/component.tsx index ee76c39742..5388a381de 100644 --- a/packages/components/src/border-box-control/border-box-control-linked-button/component.tsx +++ b/packages/components/src/border-box-control/border-box-control-linked-button/component.tsx @@ -29,7 +29,7 @@ const BorderBoxControlLinkedButton = ( <View className={ className }> <Button { ...buttonProps } - isSmall + size="small" icon={ isLinked ? link : linkOff } iconSize={ 24 } aria-label={ label } diff --git a/packages/components/src/border-box-control/border-box-control-linked-button/hook.ts b/packages/components/src/border-box-control/border-box-control-linked-button/hook.ts index 3d535a6583..1d00d059ad 100644 --- a/packages/components/src/border-box-control/border-box-control-linked-button/hook.ts +++ b/packages/components/src/border-box-control/border-box-control-linked-button/hook.ts @@ -16,17 +16,16 @@ import type { LinkedButtonProps } from '../types'; export function useBorderBoxControlLinkedButton( props: WordPressComponentProps< LinkedButtonProps, 'button' > ) { - const { - className, - size = 'default', - ...otherProps - } = useContextSystem( props, 'BorderBoxControlLinkedButton' ); + const { className, ...otherProps } = useContextSystem( + props, + 'BorderBoxControlLinkedButton' + ); // Generate class names. const cx = useCx(); const classes = useMemo( () => { - return cx( styles.borderBoxControlLinkedButton( size ), className ); - }, [ className, cx, size ] ); + return cx( styles.borderBoxControlLinkedButton(), className ); + }, [ className, cx ] ); return { ...otherProps, className: classes }; } diff --git a/packages/components/src/border-box-control/border-box-control/component.tsx b/packages/components/src/border-box-control/border-box-control/component.tsx index dbef63eeb5..576a4ebd27 100644 --- a/packages/components/src/border-box-control/border-box-control/component.tsx +++ b/packages/components/src/border-box-control/border-box-control/component.tsx @@ -143,7 +143,6 @@ const UnconnectedBorderBoxControl = ( <BorderBoxControlLinkedButton onClick={ toggleLinked } isLinked={ isLinked } - size={ inputHeight } /> </View> </View> diff --git a/packages/components/src/border-box-control/styles.ts b/packages/components/src/border-box-control/styles.ts index 3fba981080..6007f9671a 100644 --- a/packages/components/src/border-box-control/styles.ts +++ b/packages/components/src/border-box-control/styles.ts @@ -22,12 +22,11 @@ export const wrapper = css` position: relative; `; -export const borderBoxControlLinkedButton = ( - size?: 'default' | '__unstable-large' -) => { +export const borderBoxControlLinkedButton = () => { return css` position: absolute; - top: ${ size === '__unstable-large' ? '8px' : '3px' }; + top: 50%; + transform: translateY( -50% ); ${ rtl( { right: 0 } )() } line-height: 0; `; diff --git a/packages/components/src/border-box-control/types.ts b/packages/components/src/border-box-control/types.ts index 283faf0501..a08cae1661 100644 --- a/packages/components/src/border-box-control/types.ts +++ b/packages/components/src/border-box-control/types.ts @@ -53,7 +53,7 @@ export type BorderBoxControlProps = ColorProps & __next40pxDefaultSize?: boolean; }; -export type LinkedButtonProps = Pick< BorderBoxControlProps, 'size' > & { +export type LinkedButtonProps = { /** * This prop allows the `LinkedButton` to reflect whether the parent * `BorderBoxControl` is currently displaying "linked" or "unlinked"

I think this would be a good follow-up PR so this one doesn't get too big. And because when applying the diff, I ran into an alignment issue with the link button overlapping:

I think we can remove the absolute positioning to fix this, but I can explore that separately.

packages/components/src/box-control/styles/box-control-styles.ts

ciampo · 2023-11-17T14:53:46Z

packages/components/src/box-control/styles/box-control-styles.ts

+const RootWidth = ( {
+	__next40pxDefaultSize,
+}: Pick< BoxUnitControlProps, '__next40pxDefaultSize' > ) => {
+	const maxWidth = __next40pxDefaultSize ? '320px' : '235px';


@jameskoster I believe that @brookewp was requesting feedback in particular regarding this change:

I've included some changes to the width when the prop is enabled, otherwise, the text isn't visible.

Personally, I believe there's a chance that this new width may not really come into effect when the component is used in the editor's sidebar: (this can also be tested in storybook itself)

If that's the case, we may have to think about alternative solutions to make sure that all inputs can show the text, while also fitting in the sidebar

Possibly a silly question... I understand why a min-width is required on the inputs, but why do we need a max-width? Can't they just scale?

I guess that's separate...

Inspector aside, shouldn't the component be able to scale down to smaller sizes regardless? 280px (Inspector width) is small, but not that small...

All that said, I don't know if this one component is worth prioritising – I'm not sure it's even used in core.

Apologies if I'm still misunderstanding the question.

why do we need a max-width? Can't they just scale?

That's a good point. I agree that components should rarely have a max-width (it should be the consumer of this component to add one, if needed). @brookewp , could you look into that, and in particular if (by looking at the history of the component) there's a clear reason for having the max-width ?

Inspector aside, shouldn't the component be able to scale down to smaller sizes regardless? 280px (Inspector width) is small, but not that small...
Apologies if I'm still misunderstanding the question.

Regardless of whether we should be setting a max-width in the component or not, if I understand correctly @brookewp is flagging that, when rendering the UnitControl components with the new 40px height, those inputs need to be wider in order to show their values.

And potentially, when using this component in the editor sidebar, the available width (280px - 2*16px padding) = 248px) may not be enough to have four UnitControls in their "40px" configuration, without cutting the text inside those inputs.

All that said, I don't know if this one component is worth prioritising – I'm not sure it's even used in core.

I think that BoxControl is still used in the dimensions panel (see message below).

It would be great if we could get a list sorted by priority — although ultimately I seem to remember that all components need to be refactored to the new 40px size. We'd like to wrap up the migration sooner rather than later, so that we can refactor our UIs and ultimately remove both the __next40pxDefaultSize prop and the __unstable-large size.

Gotcha. I think the point stands though – since BoxControl is not restricted to the Inspector, it should scale elegantly to smaller widths? If we wanted to reuse existing patterns, here some options:

Custom padding (when spacing presets are provided)

(The settings button would be omitted).

Border control

(The color picker would be omitted).

I don't have a strong feeling either way. As I said this component isn't used in core very often since most themes provide spacing presets nowadays. If neither of these patterns will work, or are too difficult to implement, then we'll probably need to revisit the design.

(this can also be tested in storybook itself)

Such a helpful TIL 🥲 Thank you!

could you look into that, and in particular if (by looking at the history of the component) there's a clear reason for having the max-width ?

This was part of the initial creation of the component three years ago. Nothing was mentioned in relation to the width/input size. The component was made for the Cover block, so my guess is it was sized to fit within the sidebar. So I don't think a hard width is needed.

And potentially, when using this component in the editor sidebar, the available width (280px - 2*16px padding) = 248px) may not be enough to have four UnitControls in their "40px" configuration, without cutting the text inside those inputs.

Looks like that's the case; it's not wide enough 😓

As I said this component isn't used in core very often since most themes provide spacing presets nowadays.

So it is likely there for backwards compability? Since it shows in the case when showSpacingPresetsControl is false. And it was added quite recently in #48070.

since BoxControl is not restricted to the Inspector, it should scale elegantly to smaller widths? If we wanted to reuse existing patterns, here some options:

@ciampo should we look into an alternative pattern when __next40pxDefaultSize is enabled?

So it is likely there for backwards compability?

Yes. So rather than coming up with a new design it would be simpler to re-use existing patterns (like the padding / border examples above). That way the component will work nicely in any environment, not just the Inspector. What do you think?

I believe the Custom padding option makes the most sense for BoxControl. However, are we okay with imposing a limitation on the values by includingRangeControl? cc @ciampo

If not, are we okay with the Border control option when there are only two values? cc @jameskoster

For example:

I agree with Brooke, the layout of the custom padding controls feels like a better fit — although that means that we'll have to introduce a slider next to each input.

That works for me.

ciampo · 2023-11-17T15:05:36Z

packages/components/src/box-control/types.ts

On a separate note, I realsed that BoxControl's storybook file hasn't been updated to the same format that other components use (ie. with a meta object, and using the template + args format). Which means that currently, controls are not enabled for its storybook examples. Would you be able to improve this too? (it can definitely happen in a separate PR)

ciampo · 2023-11-17T15:11:24Z

@jameskoster :

but the top border seems to be missing in the Inspector:

I believe the component is behaving as expected, since the top border in your screenshot has a width of 0px. Increasing the width should cause the border to show again

I don't think BoxControl is used anywhere, so I couldn't smoke test that one.

It looks like it is used in the dimension panel for padding/margin/block gaps, when spacing presets (which I don't know what they are) are not used

jameskoster · 2023-11-17T17:14:49Z

I believe the component is behaving as expected, since the top border in your screenshot has a width of 0px. Increasing the width should cause the border to show again

Derp, of course 🤦‍♂️

chad1008

Thanks for putting all of this together @brookewp!

I saw and agree with Marco's comment on the benefit of using __unstable-large. One thing related to that that stood out to me is that on this PR there are some style changes on the unit selector that appear to come from __unstable-large, in addition to the size itself. So that will need to be preserved when __unstable-large and __next40pxDefaultSize are both eventually deprecated.

ciampo · 2023-11-22T15:24:00Z

So, to recap, it looks like there are a few pre-requisites and/or follow-ups to this PR.

Here's a suggested plan of action:

Following this conversation, we could refactor BorderBox to find a different layout (regardless of the 40px size) and remove any max-width. I would try to optimize for whatever is the easiest/quickest solution here
In the meantime, we could:
- improve the Storybook files
- tweak BorderControl dropdown button size (and borders) as suggested here
Once that's done, we could resume work on this PR and merge it
Finally, and optionally, we could take a look at refining BorderBoxControl's styles

What do folks think?

brookewp · 2023-12-06T00:34:59Z

A status update on the prelude to this PR via the plan ciampo shared above (steps 1-2):

refactor BoxControl to find a different layout (regardless of the 40px size) and remove any max-width BoxControl: update design #56665
improve the Storybook files BoxControl: Update story and refactor to Typescript #56462
tweak BorderControl dropdown button size (and borders) BorderControl: Fix button styles #56730

ciampo · 2024-01-12T17:50:29Z

Code changes are nice and clear, and overall things are looking good.

One thing that I was wondering, is whether we should also forward the new 40px default size to the components in the dropdown of BorderControl and BorderBoxControl:

That could also happen in a follow-up, but it would mean updating a few more components to support the new sizing scheme.

(cc @jameskoster )

jameskoster · 2024-01-15T10:21:00Z

Potentially, yes! Though I would rather prioritise updating the swatch buttons to use the 32/40 sizing convention.

ciampo

I believe we can merge this PR (after rebasing, which should also fix the CI failures) 🚀

We could then follow-up on updating the componentry in BorderControl's dropdown:

Potentially, yes! Though I would rather prioritise updating the swatch buttons to use the 32/40 sizing convention.

@mirka 's work in #57562 is already potentially taking care of the "style" section in the dropdown.

@brookewp , maybe you could take a look at the swatch buttons? I guess we could:

add a size prop which adds a size="compact" (32px)
add the usual opt-in prop for chaing the default size to 40px

What do y'all think?

mirka · 2024-01-15T16:27:48Z

updating the swatch buttons to use the 32/40 sizing convention

@jameskoster Just to be clear, are there actual use cases where the CircularOptionPicker needs to be 32 in some places but 40px in others?

In most (all?) consumer components (ColorPalette, DuotonePicker, GradientPicker, PaletteEdit), I'm hoping we can just update the swatch button sizes internally to a single new size without providing multiple size options.

jameskoster · 2024-01-16T11:41:54Z

Just to be clear, are there actual use cases where the CircularOptionPicker needs to be 32 in some places but 40px in others?

It's hard to say without doing a full audit, but I'd be inclined to say they should always all be the same size. It certainly seems fine to start with a single size.

brookewp added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Nov 16, 2023

brookewp self-assigned this Nov 16, 2023

brookewp requested a review from ajitbohra as a code owner November 16, 2023 02:50

brookewp requested review from a team November 16, 2023 02:50

ciampo reviewed Nov 17, 2023

View reviewed changes

chad1008 reviewed Nov 21, 2023

View reviewed changes

brookewp mentioned this pull request Nov 23, 2023

BoxControl: Update story and refactor to Typescript #56462

Merged

This was referenced Nov 30, 2023

BoxControl: update design #56665

Merged

BorderControl: Fix button styles #56730

Merged

ciampo added this to In progress (owned) ⏳ in WordPress Components via automation Dec 13, 2023

brookewp force-pushed the add/40px-size-box-border-control branch 2 times, most recently from a23876d to 11a22e2 Compare January 12, 2024 04:34

brookewp requested review from ciampo, chad1008 and a team January 12, 2024 05:22

ciampo approved these changes Jan 15, 2024

View reviewed changes

brookewp added 4 commits January 15, 2024 10:49

Add 40px size to BoxControl

77a6020

Add 40px size to BorderControl

2d7bbf6

Add 40px size to BorderBoxControl

28092ba

Move logic to hook for BorderControl

dfe78d5

brookewp added 2 commits January 15, 2024 10:49

Move logic to hook for BorderBoxControl

ac3cdab

Update changelog

c8e7f47

brookewp force-pushed the add/40px-size-box-border-control branch from 11a22e2 to c8e7f47 Compare January 15, 2024 18:50

brookewp merged commit 464961e into trunk Jan 15, 2024
56 checks passed

WordPress Components automation moved this from In progress (owned) ⏳ to Done 🎉 Jan 15, 2024

brookewp deleted the add/40px-size-box-border-control branch January 15, 2024 23:54

github-actions bot added this to the Gutenberg 17.6 milestone Jan 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Components: Add opt-in prop for 40px default size for `BoxControl`, `BorderControl`, and `BorderBoxControl` #56185

Components: Add opt-in prop for 40px default size for `BoxControl`, `BorderControl`, and `BorderBoxControl` #56185

brookewp commented Nov 16, 2023 •

edited

Loading

jameskoster commented Nov 16, 2023

ciampo left a comment

ciampo Nov 17, 2023

brookewp Nov 21, 2023

ciampo Nov 17, 2023

jameskoster Nov 17, 2023

ciampo Nov 17, 2023 •

edited

Loading

jameskoster Nov 20, 2023

brookewp Nov 21, 2023

brookewp Nov 21, 2023

jameskoster Nov 21, 2023

brookewp Nov 24, 2023

ciampo Nov 24, 2023

jameskoster Nov 24, 2023

ciampo Nov 17, 2023

ciampo commented Nov 17, 2023

jameskoster commented Nov 17, 2023

chad1008 left a comment

ciampo commented Nov 22, 2023 •

edited

Loading

brookewp commented Dec 6, 2023 •

edited

Loading

ciampo commented Jan 12, 2024 •

edited

Loading

jameskoster commented Jan 15, 2024

ciampo left a comment

mirka commented Jan 15, 2024

jameskoster commented Jan 16, 2024

	{ ! showSpacingPresetsControl && (
	<BoxControl
	values={ paddingValues }
	onChange={ setPaddingValues }
	label={ __( 'Padding' ) }
	sides={ paddingSides }
	units={ units }
	allowReset={ false }
	splitOnAxis={ isAxialPadding }
	onMouseOver={ onMouseOverPadding }
	onMouseOut={ onMouseLeaveControls }
	/>
	) }

Components: Add opt-in prop for 40px default size for BoxControl, BorderControl, and BorderBoxControl #56185

Components: Add opt-in prop for 40px default size for BoxControl, BorderControl, and BorderBoxControl #56185

Conversation

brookewp commented Nov 16, 2023 • edited Loading

What?

Why?

How?

BoxControl

Testing Instructions

In Storybook:

In the editor

jameskoster commented Nov 16, 2023

ciampo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Custom padding (when spacing presets are provided)

Border control

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo commented Nov 17, 2023

jameskoster commented Nov 17, 2023

chad1008 left a comment

Choose a reason for hiding this comment

ciampo commented Nov 22, 2023 • edited Loading

brookewp commented Dec 6, 2023 • edited Loading

ciampo commented Jan 12, 2024 • edited Loading

jameskoster commented Jan 15, 2024

ciampo left a comment

Choose a reason for hiding this comment

mirka commented Jan 15, 2024

jameskoster commented Jan 16, 2024

Components: Add opt-in prop for 40px default size for `BoxControl`, `BorderControl`, and `BorderBoxControl` #56185

Components: Add opt-in prop for 40px default size for `BoxControl`, `BorderControl`, and `BorderBoxControl` #56185

brookewp commented Nov 16, 2023 •

edited

Loading

`BoxControl`

ciampo Nov 17, 2023 •

edited

Loading

ciampo commented Nov 22, 2023 •

edited

Loading

brookewp commented Dec 6, 2023 •

edited

Loading

ciampo commented Jan 12, 2024 •

edited

Loading