Repository navigation
Conversation
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
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. |
| // Legacy default height (36px) has no exact WPDS size token match. | ||
| --wp-components-button-height: 36px; |
There was a problem hiding this comment.
Not sure if there's an alternative solution 🤔
|
Size Change: +435 B (+0.01%) Total Size: 7.75 MB 📦 View Changed
|
|
Flaky tests detected in b87b609. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/29116506316
|
This comment was marked as outdated.
This comment was marked as outdated.
ciampo
left a comment
There was a problem hiding this comment.
I'm torn on whether we should continue to use the SASS variables or switch to WPDS tokens directly. if the goal is true @wordpress/ui alignment, we should migrate Button internals toward WPDS tokens directly. If the goal is legacy compatibility, keep component aliases or use local Button variables that bridge both, eg
--wp-components-button-background-color: var(
--wp-components-color-accent,
var(--wpds-color-background-interactive-brand-strong)
);That gives a migration path without dropping existing customization hooks too abruptly.
Re. background: transparent, I guess that we should generally map to WPDS weak background tokens (instead of raw transparent) when they represent a button tone/variant.
We should also be careful when replacing background: none with a color token. background: none resets images too, which matters for busy/loading styles.
Here's a potential mapping
components primary
-> ui brand solid
bg: --wpds-color-background-interactive-brand-strong
bg active: --wpds-color-background-interactive-brand-strong-active
fg: --wpds-color-foreground-interactive-brand-strong
components secondary
-> ui brand outline
bg: --wpds-color-background-interactive-brand-weak // #0000
bg active: --wpds-color-background-interactive-brand-weak-active
bg disabled: --wpds-color-background-interactive-brand-weak-disabled
border: --wpds-color-stroke-interactive-brand
fg: --wpds-color-foreground-interactive-brand
components tertiary
-> ui brand minimal
bg: --wpds-color-background-interactive-brand-weak // #0000
bg active: --wpds-color-background-interactive-brand-weak-active
bg disabled: --wpds-color-background-interactive-brand-weak-disabled
fg: --wpds-color-foreground-interactive-brand
destructive secondary/tertiary
-> error outline/minimal equivalent
bg: --wpds-color-background-interactive-error-weak // #0000
bg active: --wpds-color-background-interactive-error-weak-active
fg: --wpds-color-foreground-interactive-error
border: --wpds-color-stroke-interactive-error for outline-ish secondaryThis is a change with a potential large visual impact, probably better to land it after the next Gutenberg release (which means this change will be included in WP 7.2, giving us ample time to test and make adjustments if needed)
Also cc @t-hamano as some the folks with the keenest eye on Button.
| // TODO: Prepare for theming (https://github.com/WordPress/gutenberg/pull/45466/files#r1030872724) | ||
| color: rgba($white, 0.4); | ||
| color: var(--wpds-color-foreground-interactive-brand-strong-disabled); |
There was a problem hiding this comment.
This is a potentially substantial difference. More in general, would you mind updating a full matrix of the button variants, like done in #78646 ?
We have a specific Storybook example used for ad-hoc e2e viz reg that you can visit by running npm run storybook:e2e:dev
There was a problem hiding this comment.
Thanks for the tip about strokybook:e2e! Didn't know we had more examples to test with there.
I'm assuming some visual changes are ok considering what we just did with Notice.
Primary+disabled button:
Components Button in trunk (looks better):

So I think we should change the background and text colour in tandem, at least for the disabled state, to avoid some kind of mixed style between the two that doesn't look that great.
There was a problem hiding this comment.
Yes, although I believe this warrants a broader conversation (to make sure we potentially address button styles on the php side) and the blessing of @WordPress/gutenberg-design
There was a problem hiding this comment.
Catching up. What needs a decision here? Is it the disabled state? If yes then I agree trunk looks better, gray on blue is barely legible (even though it doesn't have to be on a disabled button).
Noting that there was a previous attempt to move disabled buttons to look substantially less prominent in #70674, which was reverted specifically because we wanted to move towards a more considered token-based system. In that range, does this move us in the right direction? If it's mostly the disabled states, let me know and I'll test more in-depth those styles.
There was a problem hiding this comment.
@jasmussen, in the UI-Button disabled state looks like this:
Can we make the components-Button look the same, or is it better to keep it as-is today (image below) and migrate to the new disabled style by swapping out components around Gutenberg one-by-one?
There was a problem hiding this comment.
Thanks for the clarity. I feel like this is moving in the right direction, on all counts, and is a good argument for why we made the origiginal change. It's exactly what we said we'd do: do it systematically, with tokens. Kudos.
Can we make the components-Button look the same, or is it better to keep it as-is today (image below) and migrate to the new disabled style by swapping out components around Gutenberg one-by-one?
That's a good question. In principle that would be ideal since they are meant to look the same. But perhaps it's okay for this to be a slow transition with the old still keeping the legacy behavior? How do you feel about it?
There was a problem hiding this comment.
We did mention that, ideally, we would minimize the visual difference between @wordpress/components and @wordpress/ui componentry to ensure a better transition.
IMO, we should prioritize refactoring the styles in @wordpress/components to follow the new look from @wordpress/ui, since it will allow us to mix-and-match the two components (legacy and new version) without too many inconsistencies as we progressively refactor the repo (and as third party devs may continue to use the legacy version)
f9a939d to
b87b609
Compare
|
I updated this PR to touch only font size, border width, spacing and things like that; I'm moving all the colours-related token swapping to happen at once in the follow-up PR #79994 I'll update PR description with screenshots. |
b87b609 to
78bd5e9
Compare
e8becb6 to
e5a84a5
Compare
| // 22px has no exact WPDS line-height token match (sm is 20px, md is 24px). | ||
| line-height: 22px; |
There was a problem hiding this comment.
UI uses --wpds-typography-line-height-sm (20px) for all sizes, including small.
UI small = 24px tall, 13px font (font-size-md), 20px line-height, 0 vertical padding.
Components small = 24px tall, 11px font (font-size-xs), 22px line-height, 0 vertical padding.
Both end up 24px tall.
There was a problem hiding this comment.
...separate question if we should align the font sizes for the small variant. Could be a good follow-up or done already here.
Default and compact use already the same font size.
There was a problem hiding this comment.
Outer size matches, although typography, internal spacing and "growth" behaviour are different.
I feel like the line height shouldn't be what guarantees the minimum clickable areas (24px), and therefore I prefer the wordpress/ui approach that uses min-height. Moreover, I believe that changing line-height here won't change the clickable box.
Still, I believe we could align font-size and line height here (ie. use font-size-md and line-height-sm) , I guess we could do it in this PR?
We will need to update the matrix screenshots, since small buttons will grow wider (not taller)
There was a problem hiding this comment.
Still, I believe we could align font-size and line height here (ie. use font-size-md and line-height-sm) , I guess we could do it in this PR?
Let's do it. 👍 It's unavoidable anyway if we want to be able to mix components and have things look consistent.
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmm link button in small format now inherits wrong padding, need to fix that.
There was a problem hiding this comment.
Sounds good! Let me know once you've fixed that and I'll take another look
There was a problem hiding this comment.
Not super pretty but simply avoiding resetting the padding to 0: dc954ec
There was a problem hiding this comment.
Hmm, I can see some regressions with small link buttons height:
Excluding .is-link from both height and padding fixes the horizontal padding, but it makes the small link target depend on its content. With the current cascade, a text-only small link is 20px tall, an icon-only one is 24px, and an icon-and-text one is 36px because the later .has-icon rule restores 6px block padding around the 24px icon. Trunk kept all three at 24px. Could we guarantee the 24px small control height for every variant and special-case only the link padding? The small link-and-icon case also needs to avoid reacquiring that later block padding.
5844afe to
5872476
Compare
@ciampo, agreed on this — which exact GB version is safe for Button changes? |
Probably 23.7? (@t-hamano can confirm) |
This PR will not be backported to WP 7.1, correct? If so, there is nothing to do. Merging this PR now will release it as part of GB 23.7. |
| // 22px has no exact WPDS line-height token match (sm is 20px, md is 24px). | ||
| line-height: 22px; |
There was a problem hiding this comment.
Outer size matches, although typography, internal spacing and "growth" behaviour are different.
I feel like the line height shouldn't be what guarantees the minimum clickable areas (24px), and therefore I prefer the wordpress/ui approach that uses min-height. Moreover, I believe that changing line-height here won't change the clickable box.
Still, I believe we could align font-size and line height here (ie. use font-size-md and line-height-sm) , I guess we could do it in this PR?
We will need to update the matrix screenshots, since small buttons will grow wider (not taller)
037c139 to
dc954ec
Compare
dc954ec to
5abb944
Compare
|
Now that 7.1 shipped, should we resume work and aim to ship this soon into the 7.2 dev cycle? Noting that, in the meantime, we've unified focus rings already |





What?
Part of #76135
@wordpress/componentsButton baseline styles to WPDS tokens (typography, spacing, sizing, radius, and focus), while preserving existing component behaviour and legacy 36px default height.Doesn't migrate colour-related variables yet to keep testing easier/more focused. I have another follow-up which migrates colors: #79994
Earlier PRs on Button tokenisation:
Why?
Aligns Theming systems between
@wordpress/componentsand@wordpress/ui.How?
Testing Instructions
Run storybook:
npm run storybook:devand openhttp://localhost:50240/?path=/docs/components-button--docsVerify Button variants in Storybook (
default,primary,secondary,tertiary,link,small,compact, icon-only) all look and behave the same:Testing Instructions for Keyboard
Screenshots or screencast
There should not be any visual difference between before/after:
Before
After
Icon one specifically:
Before
After
Use of AI Tools
yes