-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: +1.29 kB (+0.07%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
addbac0
to
01db75a
Compare
* | ||
* @return {Element} Custom border radius control. | ||
*/ | ||
export default function BorderRadiusControl( { onChange, values } ) { | ||
export default function BorderRadiusControl( { onChange, values, presets } ) { |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I have no idea why they look different :P I just copied the code :)
I'll check again but it's also possible that the CSS you had was outdated/cached no? |
@jasmussen I found an issue if you didn't have any preset (the default case), now we need to understand why your presets are not being picked up. |
@jasmussen Are you certain the right theme is enabled? (It happened to me and spent sometime wondering why I'm not seeing the presets) |
Flaky tests detected in d01da07. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12194039530
|
I added the icons suggested by @jameskoster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm excited to see this enhancement happening, thanks @youknowriad 🙇
So far, I've only had the chance to give the code a cursory glance and start to put the control and presets through their paces in the editor. A number of issues popped up pretty quickly but I'm hoping they won't take too much to sort out.
Presets do not show in the Global Styles border panel but do in the block inspector panel- It looks like the global styles panel is receiving settings that don't include the
settings.border.radiusSizes
setting. - Fixed in ee37bda
- It looks like the global styles panel is receiving settings that don't include the
- After adding presets to theme.json, the control is just broken for me
- Clicking the initial position on the slider shows empty tooltip
- From there I can't adjust the slider at all
- After unlinking the control, I can move the individual sliders however the sliders show more segments than there are presets (I was just using the sample presets in PR description)
- If a
0
border radius was selected it will show as a custom value rather than thenone
preset that always gets added to the slider options.
There's some errors thrown due to(fixed 21b032d)fill-rule
andclip-rule
in the icons
Screen.Recording.2024-12-05.at.12.59.50.pm.mp4
I see a lot of the mouse over handling etc is commented out. Was this for testing purposes or is the intent to remove that behaviour eventually?
Later today, I'll be able to dig a bit deeper and can push some smaller fixes if that helps.
const sizes = [ | ||
{ name: __( 'None' ), slug: '0', size: 0 }, | ||
...customSizes, | ||
...themeSizes, | ||
...defaultSizes, | ||
]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
).
This explains part of what I was seeing while testing.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This all might be replaced with extracted generic utils.
The style engine config might also need updating for the new |
@aaronrobertshaw I think that config is probably for dynamic blocks. |
I thought so too but without updating the style engine config, the preset styles still appeared to work for the image block. Looks like I needed to cast the net wider. Without updating the style engine config the presets don't work for the Site Title block on the frontend. After tweaking that, the show up correctly. I've pushed that in aa60fa3. With all that sorted, the "linked" control handling "all" sides is still broken. I'm out of time today but can take another look tomorrow if it helps. |
I fixed the control in global styles in the last commit. (It's so convoluted 😬) |
0361669
to
bd11946
Compare
@jasmussen Ok, so I just found that we have some custom CSS that overrides the style of the "marks" in the Range Control for the spacing sizes. This is why the range control looks different in the "padding/margin" compared to the regular control provided by @wordpress/components We have a few options: 1- Do the same CSS override for the control used for border radius.
Curious about your thoughts. |
Ok options 2 and 3 combined are being implemented here #67611 |
Nice work. It seems like there's a broader question around the two styles of sliders, that would be good to not block this one. Since it uses the same slider style as Padding and Margin, this is 👍 👍 to go with another review! Nice work, this'll be a potent feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to test this after the bug fixes last night. Thanks @youknowriad and @aaronrobertshaw!
So far for me:
🍏 Theme.json presets work, testing main units (px, rem, em)
🍏 Global styles
🍏 Block level styles.
🍏 Tested with server-rendered and inline block support styles, including gallery, image, site title, group, button
path: [ 'border', 'radiusSizes' ], | ||
valueKey: 'size', | ||
cssVarInfix: 'border-radius', | ||
valueFunc: ( { size } ) => size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure the valueFunc
isn't necessary as no special value processing is taking place, so it'll just grab the value using the valueKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for flagging that. Looks like we should also remove valueFunc
from the spacingSizes
config above too.
@@ -1,26 +1,13 @@ | |||
.components-border-radius-control__header { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also give the radius range control marks the same style as the spacing one? White horizontal marks instead of grey.
.components-range-control__mark { |
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're making progress here 🚀
While re-testing this today I've run into a couple of further issues with the new control.
Invalid Global Styles generated
- Without border radius presets or configuring custom radii
- Navigate to a block with radius support and no global styles, leave the unit control in its empty initial state
- Drag slider
- Inspect the block's element in dev tools and note that the control provides an
undefined
value for the CSS unit.
Custom values sliders don't work
- Without border radius presets or after switching to "custom" mode for the control
- Without any value in the unit control, drag the slider and note no radius gets applied to the block
I suspect both of these behaviours are the same issue i.e. we're not handling the default unit correctly.
Fixes #64041
What
This PR adds support for "border radius presets" to theme.json and allow picking border radius using these presets in the different controls. It uses the "spacing sizes" as a model to implement this.
It looks like this for the moment (pending the addition of icons)
Testing instructions