Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Themes: Support providing border radius presets #67544

Open
wants to merge 13 commits into
base: trunk
Choose a base branch
from
Open

Themes: Support providing border radius presets #67544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e0b4e47
Global Styles: Introduce border radius presets
youknowriad Dec 3, 2024
01db75a
Allow picking presets
youknowriad Dec 3, 2024
875dce4
Remove debug code
youknowriad Dec 4, 2024
f3b4abe
Fix no presets case
youknowriad Dec 4, 2024
92df5cf
Add icons
youknowriad Dec 4, 2024
21b032d
Fix invalid DOM property errors in new icons
aaronrobertshaw Dec 5, 2024
e7b603d
Fix CSS custom property name in schema description
aaronrobertshaw Dec 5, 2024
ee37bda
Add radiusSizes to valid settings in global styles
aaronrobertshaw Dec 5, 2024
9bb6c03
Tweak copy paste references to spacing
aaronrobertshaw Dec 5, 2024
7ee1523
Add missing global styles config for radiusSizes
aaronrobertshaw Dec 5, 2024
aa60fa3
Update style engine for radius presets
aaronrobertshaw Dec 5, 2024
bd11946
Fix global styles preset control
youknowriad Dec 5, 2024
d01da07
Fix linting error
aaronrobertshaw Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/reference-guides/theme-json-reference/theme-json-living.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ Settings related to borders.
| radius | Allow users to set custom border radius. | `boolean` | `false` |
| style | Allow users to set custom border styles. | `boolean` | `false` |
| width | Allow users to set custom border widths. | `boolean` | `false` |
| radiusSizes | Border radius size presets for the border radius selector. | `[ { name, slug, size } ]` | |

---

Expand Down
18 changes: 14 additions & 4 deletions lib/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,15 @@ class WP_Theme_JSON_Gutenberg {
'classes' => array(),
'properties' => array( 'box-shadow' ),
),
array(
'path' => array( 'border', 'radiusSizes' ),
'prevent_override' => false,
'use_default_names' => false,
'value_key' => 'size',
'css_vars' => '--wp--preset--border-radius--$slug',
'classes' => array(),
'properties' => array( 'border-radius' ),
),
);

/**
Expand Down Expand Up @@ -386,10 +395,11 @@ class WP_Theme_JSON_Gutenberg {
'backgroundSize' => null,
),
'border' => array(
'color' => null,
'radius' => null,
'style' => null,
'width' => null,
'color' => null,
'radius' => null,
'style' => null,
'width' => null,
'radiusSizes' => null,
),
'color' => array(
'background' => null,
Expand Down
14 changes: 14 additions & 0 deletions lib/theme-i18n.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@
}
]
},
"border": {
"radiusSizes": [
{
"name": "Border radius size name"
}
]
},
"blocks": {
"*": {
"typography": {
Expand Down Expand Up @@ -77,6 +84,13 @@
"name": "Space size name"
}
]
},
"border": {
"radiusSizes": [
{
"name": "Border radius size name"
}
]
}
}
}
Expand Down
67 changes: 0 additions & 67 deletions packages/block-editor/src/components/border-radius-control/all-input-control.js
View file
Open in desktop

This file was deleted.

136 changes: 69 additions & 67 deletions packages/block-editor/src/components/border-radius-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,54 +3,71 @@
*/
import {
BaseControl,
RangeControl,
__experimentalParseQuantityAndUnitFromRawValue as parseQuantityAndUnitFromRawValue,
__experimentalUseCustomUnits as useCustomUnits,
__experimentalVStack as VStack,
__experimentalHStack as HStack,
} from '@wordpress/components';
import { useState } from '@wordpress/element';
import { useState, useMemo } from '@wordpress/element';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import AllInputControl from './all-input-control';
import InputControls from './input-controls';
import LinkedButton from './linked-button';
import { useSettings } from '../use-settings';
import {
getAllValue,
getAllUnit,
hasDefinedValues,
hasMixedValues,
} from './utils';
import { hasDefinedValues, hasMixedValues } from './utils';
import SingleInputControl from './single-input-control';

const DEFAULT_VALUES = {
topLeft: undefined,
topRight: undefined,
bottomLeft: undefined,
bottomRight: undefined,
};
const MIN_BORDER_RADIUS_VALUE = 0;
const MAX_BORDER_RADIUS_VALUES = {
px: 100,
em: 20,
rem: 20,
};
const RANGE_CONTROL_MAX_SIZE = 8;
const EMPTY_ARRAY = [];
function useBorderRadiusSizes( presets ) {
const defaultSizes = presets?.default ?? EMPTY_ARRAY;
const customSizes = presets?.custom ?? EMPTY_ARRAY;
const themeSizes = presets?.theme ?? EMPTY_ARRAY;

return useMemo( () => {
const sizes = [
{ name: __( 'None' ), slug: '0', size: 0 },
...customSizes,
...themeSizes,
...defaultSizes,
];
Comment on lines +36 to +41
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struck me as a little odd. Is this back-to-front as it appears to spread user, theme, then default sizes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've probably gotten lost along the way but it appears there was custom merging for spacing sizes added in #61842 while adding support for a defaultSpacingSizes option. This approach moved merged spacing sizes under an origin key.

It doesn't appear that has been implemented for radius sizes and might be a factor in some of the bugs encountered in testing.

At this stage, it doesn't look like we'll add default border radii to the core theme.json. Without that, we probably don't need a border-radius equivalent to defaultSpacingSizes or custom merging of presets under origin keys.

If I'm wrong about whether we'll add default border radii presets to core theme.json, implementing a defaultRadiusSizes option and merging might require a theme.json version bump like the spacing changes needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 Nevermind me. I see now where the presets are keyed by origin.

My questions now are:

  • Should we be supporting a defaultRadiusSizes option?
  • Do radius sizes need merging in the same fashion as spacing sizes?
  • Shouldn't the merge order here be default, theme, then custom/user?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, when default radius sizes are defined within lib/theme.json (or via get_core_data filter) and a theme redefines any of those sizes using the same slug both are included in the sizes options calculated here. That issue might be exacerbated if we ever allowed user origin definitions (customSizes).

Screenshot 2024-12-05 at 3 45 22 pm

This explains part of what I was seeing while testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be supporting a defaultRadiusSizes option?

I don't know, wanted to start small. I'll defer to designers

Do radius sizes need merging in the same fashion as spacing sizes?

What does this mean?

Shouldn't the merge order here be default, theme, then custom/user?

I think I copied from the spacing, I guess the idea is that the user ones appears first in the list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, wanted to start small. I'll defer to designers

Fair enough 👍

What does this mean?

I meant that if the different origins contain a preset item with the same slug, the incoming value overrides the original as the spacing presets do here.

I think I copied from the spacing, I guess the idea is that the user ones appears first in the list.

That makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that if the different origins contain a preset item with the same slug, the incoming value overrides the original as the spacing presets do here.

Why is this done in the server and not in the client for spacing sizes? This seems like it will hide presets from the global styles UI (assuming one can edit the presets...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's context I don't have yet, I'm afraid. My hunch is that it might be due to the calculation of spacing presets from the spacingScale setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even that computation shouldn't be done entirely on the server.


return sizes.length > RANGE_CONTROL_MAX_SIZE
? [
{
name: __( 'Default' ),
slug: 'default',
size: undefined,
},
...sizes,
]
: sizes;
}, [ customSizes, themeSizes, defaultSizes ] );
}

/**
* Control to display border radius options.
*
* @param {Object} props Component props.
* @param {Function} props.onChange Callback to handle onChange.
* @param {Object} props.values Border radius values.
* @param {Object} props.presets Border radius presets.
*
* @return {Element} Custom border radius control.
*/
export default function BorderRadiusControl( { onChange, values } ) {
export default function BorderRadiusControl( { onChange, values, presets } ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component was a nightmare to refactor and update to support the presets. I'm really surprised about this and thought by this point, we'd have most of the UI components covered. I tried to replicate what we do for the "spacing sizes" a bit but I believe that component should be extracted and unified as a generic UI component for any "property" that has "sides/corders" and supports "presets".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From memory, this border radius component followed the original box control component although it had some different requirements like layout and an in-flux design. I have a feeling the spacing sizes component followed a similar path but evolved differently too.

The proposed generic UI component sounds good but whoever picks it up might face similar friction.

const [ isLinked, setIsLinked ] = useState(
! hasDefinedValues( values ) || ! hasMixedValues( values )
);

const options = useBorderRadiusSizes( presets );
// Tracking selected units via internal state allows filtering of CSS unit
// only values from being saved while maintaining preexisting unit selection
// behaviour. Filtering CSS unit only values prevents invalid style values.
Expand All @@ -72,64 +89,49 @@ export default function BorderRadiusControl( { onChange, values } ) {
availableUnits: availableUnits || [ 'px', 'em', 'rem' ],
} );

const unit = getAllUnit( selectedUnits );
const unitConfig = units && units.find( ( item ) => item.value === unit );
const step = unitConfig?.step || 1;

const [ allValue ] = parseQuantityAndUnitFromRawValue(
getAllValue( values )
);

const toggleLinked = () => setIsLinked( ! isLinked );

const handleSliderChange = ( next ) => {
onChange( next !== undefined ? `${ next }${ unit }` : undefined );
};

return (
<fieldset className="components-border-radius-control">
<BaseControl.VisualLabel as="legend">
{ __( 'Radius' ) }
</BaseControl.VisualLabel>
<div className="components-border-radius-control__wrapper">
{ isLinked ? (
<>
<AllInputControl
className="components-border-radius-control__unit-control"
values={ values }
min={ MIN_BORDER_RADIUS_VALUE }
onChange={ onChange }
selectedUnits={ selectedUnits }
setSelectedUnits={ setSelectedUnits }
units={ units }
/>
<RangeControl
__next40pxDefaultSize
label={ __( 'Border radius' ) }
hideLabelFromVision
className="components-border-radius-control__range-control"
value={ allValue ?? '' }
min={ MIN_BORDER_RADIUS_VALUE }
max={ MAX_BORDER_RADIUS_VALUES[ unit ] }
initialPosition={ 0 }
withInputField={ false }
onChange={ handleSliderChange }
step={ step }
__nextHasNoMarginBottom
/>
</>
) : (
<InputControls
min={ MIN_BORDER_RADIUS_VALUE }
<HStack className="components-border-radius-control__header">
<BaseControl.VisualLabel as="legend">
{ __( 'Radius' ) }
</BaseControl.VisualLabel>
<LinkedButton onClick={ toggleLinked } isLinked={ isLinked } />
</HStack>
{ isLinked ? (
<>
<SingleInputControl
onChange={ onChange }
selectedUnits={ selectedUnits }
setSelectedUnits={ setSelectedUnits }
values={ values || DEFAULT_VALUES }
values={ values }
units={ units }
corner="all"
presets={ options }
/>
) }
<LinkedButton onClick={ toggleLinked } isLinked={ isLinked } />
</div>
</>
) : (
<VStack>
{ [
'topLeft',
'topRight',
'bottomLeft',
'bottomRight',
].map( ( corner ) => (
<SingleInputControl
key={ corner }
onChange={ onChange }
selectedUnits={ selectedUnits }
setSelectedUnits={ setSelectedUnits }
values={ values || DEFAULT_VALUES }
units={ units }
corner={ corner }
presets={ options }
/>
) ) }
</VStack>
) }
</fieldset>
);
}
Loading
Loading