Refactor Admin UI / Breadcrumbs to use DS components and design tokens#77012
Conversation
|
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. |
| const ALLOWLIST = { | ||
| '@wordpress/ui': { | ||
| allowed: [ 'Badge', 'Stack' ], | ||
| allowed: [ 'Badge', 'Link', 'Stack', 'Text' ], |
There was a problem hiding this comment.
If this needs to be handled separately I understand :)
There was a problem hiding this comment.
Just flagging that Text is now allowed, and I guess Link may soon allowed too?
There was a problem hiding this comment.
My expectation is that we're okay to make this allowed, but that we'd likely want to accompany this (separately, perhaps) with a more thorough reflection of its status like we did in #77044 (e.g. updating componentStatus of @wordpress/components ExternalLink to point to @wordpress/ui if appropriate).
There was a problem hiding this comment.
I'm happy to wait on merging this until Link is allowed in a separate PR if y'all prefer.
There was a problem hiding this comment.
I think focus styles reconciliation may be a blocker, also highlighted in #76135 as incomplete.
Putting together a quick example locally in Storybook, the two links have obvious differences side-by-side:
(Left is @wordpress/ui Link, right is @wordpress/components ExternalLink)
Storybook code
// packages/ui/src/link/stories/focus-ring-comparison.story.tsx
import type { Meta, StoryObj } from '@storybook/react-vite';
import { ExternalLink } from '@wordpress/components';
import { Link } from '../index';
const meta: Meta< typeof Link > = {
title: 'Design System/Components/Link/Focus ring comparison',
component: Link,
};
export default meta;
type Story = StoryObj< typeof Link >;
export const SideBySide: Story = {
render: () => (
<>
<Link href="#" openInNewTab>
WordPress.org
</Link>
<ExternalLink href="#">WordPress.org</ExternalLink>
</>
),
};Aside from focus, another question is color. Do we want to backport the coloring we've applied to Link?
There was a problem hiding this comment.
Appears to be a case of applying radius to Link (or removing it from ExternalLink)?
I suppose we ought to decide whether or not to use focus-visible here as well. As a mouse/trackpad user I find the focus ring appearing on click a bit distracting. Maybe that's separate though.
Do we want to backport the coloring we've applied to Link?
Probably makes sense. I suppose the underline styling should match too.
There was a problem hiding this comment.
By looking at the screenshot, the new Link uses the theme primary color, while the legacy ExternalLink seems to be using the default system color for focus rings (which happens to be a similar shade of blue)
|
Size Change: -27 B (0%) Total Size: 7.82 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in 6cd0da7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25048073902
|
aduth
left a comment
There was a problem hiding this comment.
This looks like it's in pretty good shape, but needs a rebase to remove the pieces that have been addressed separately for allowlisting Text usage and handling jsx-a11y/heading-has-content.
b1af8da to
c950341
Compare
4710a2f to
1543b4c
Compare
| <Text | ||
| variant="body-lg" | ||
| render={ | ||
| <Link | ||
| tone="neutral" | ||
| render={ <RouterLink to={ item.to } /> } | ||
| /> | ||
| } | ||
| > | ||
| { item.label } | ||
| </Text> |
There was a problem hiding this comment.
Have we discussed or landed on consensus around Link component from @wordpress/route using Link from @wordpress/ui ?
(Something to address separately)
| const ALLOWLIST = { | ||
| '@wordpress/ui': { | ||
| allowed: [ 'Badge', 'Stack' ], | ||
| allowed: [ 'Badge', 'Link', 'Stack', 'Text' ], |
There was a problem hiding this comment.
My expectation is that we're okay to make this allowed, but that we'd likely want to accompany this (separately, perhaps) with a more thorough reflection of its status like we did in #77044 (e.g. updating componentStatus of @wordpress/components ExternalLink to point to @wordpress/ui if appropriate).
4115aa8 to
4a12b98
Compare
|
This PR needs a rebase after #77088 landed on |
4a12b98 to
0bcd1a2
Compare
ciampo
left a comment
There was a problem hiding this comment.
We can probably remove @wordpress/base-styles from the list of dependencies in packages/admin-ui/package.json.
We will also need to update the list of ESLint suppressions, since we removed some legacy UI components usage:
npm run lint:js:prune-suppressions
git add tools/eslint/suppressions.json
git commit -m "Breadcrumbs: Prune now-unused ESLint suppressions"Finally, a minor suggestion: the PR description looks out of date (Text was already allowed-listed in a separate PR).
There was a problem hiding this comment.
I believe we can un-nest the .current and .separator styles from within .list, since CSS modules now ensures class name uniqueness
| /* @TODO: Remove truncation styles once `Text` supports truncation natively. */ | ||
| .current { | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; | ||
| } |
There was a problem hiding this comment.
@jameskoster how is truncation supposed to work? I quckly tried to resize the browser but I couldn't get the text to truncate.
We may need to add min-width: 0 on the li elements, since they currenty won't truncate as they are flexbox items
There was a problem hiding this comment.
Fixed this, but it's a rudimentary solution. We'll need something more comprehensive eventually. Coss UI Breadcrumb could be an example to follow.
There was a problem hiding this comment.
Yup — feel free to suggest any changes to #77039 that not already mentioned
| 'Card', | ||
| 'Collapsible', | ||
| 'CollapsibleCard', | ||
| 'Link', |
There was a problem hiding this comment.
This should probably get a separate CHANGELOG entry in @wordpress/ui, too?
Also, looping @mirka in to make sure we're ready to mark it as allowed, and/or if we're happy to do it as part of this PR
There was a problem hiding this comment.
The only remaining issue with Link is what @aduth noted here. I wouldn't say this is a serious blocker for a component like Link, so I'll leave it up to @jameskoster to decide whether these discrepancies are fine for now, and can be fixed later.
There was a problem hiding this comment.
I think this is a minor point that can be addressed in a follow-up.
Refactor breadcrumb links to compose the UI Link with the router Link via the render prop, adopting design-system styling. Replace the base-styles spacing variable with a design token and add Link to the eslint allowlist for @wordpress/ui. Made-with: Cursor
Made-with: Cursor
Replace HStack and Heading from @wordpress/components with Stack and Text from @wordpress/ui. Add Text to the eslint allowlist for @wordpress/ui. Made-with: Cursor
Made-with: Cursor
Use an outer Text with body-lg on the nav element so links and separators inherit typography via CSS. The inner Text with heading-lg on the h1 overrides for the current item. Removes manual font declarations from the stylesheet. Made-with: Cursor
No longer needed after #77073. Made-with: Cursor
Wrap each Link in Text with body-lg via nested render props (Text → Link → RouterLink) rather than using an outer Text on the nav element. Remove the h1 margin override. Made-with: Cursor
Ensures the separator inherits the same typography styles as the adjacent link items by rendering it as a `<Text variant="body-lg">` instead of an `li::after` pseudo-element. Made-with: Cursor
Made-with: Cursor
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Made-with: Cursor
CSS modules scope class names automatically, so nesting under .list was unnecessary. Also rename `.admin-ui-breadcrumbs__separator` to `.separator` to match the `styles.separator` lookup in the component, which was previously returning undefined. Made-with: Cursor
Flex items default to `min-width: auto`, so the <li> wrapping the current item never shrinks below its content size and `text-overflow: ellipsis` on `.current` never engages. Add `min-width: 0` to the last <li> so the truncation styles can take effect. Also set `flex-shrink: 0` on preceding items so their text doesn't wrap to multiple lines when the container is narrow. Made-with: Cursor
Replace the hardcoded 32px with `calc(var(--wpds-dimension-base) * 8)` so the value tracks the design system's base unit. A @todo is left to swap this for a dedicated `size` token once that category exists in the theme package. Made-with: Cursor
942c7bf to
6cd0da7
Compare
|
I believe we're good to merge here. @jameskoster , will you be able to follow-up on on Finally, flagging that we're discussing the next major steps for |
#77012) * Breadcrumbs: Use Link from @wordpress/ui Refactor breadcrumb links to compose the UI Link with the router Link via the render prop, adopting design-system styling. Replace the base-styles spacing variable with a design token and add Link to the eslint allowlist for @wordpress/ui. Made-with: Cursor * Breadcrumbs: Use regular font weight for separator Made-with: Cursor * Breadcrumbs: Use Stack and Text from @wordpress/ui Replace HStack and Heading from @wordpress/components with Stack and Text from @wordpress/ui. Add Text to the eslint allowlist for @wordpress/ui. Made-with: Cursor * Breadcrumbs: Adjust separator color and remove font-weight override Made-with: Cursor * Update changelog * Remove h1 margin * Breadcrumbs: Wrap nav in Text for inherited typography Use an outer Text with body-lg on the nav element so links and separators inherit typography via CSS. The inner Text with heading-lg on the h1 overrides for the current item. Removes manual font declarations from the stylesheet. Made-with: Cursor * Breadcrumbs: Remove unnecessary eslint-disable for heading-has-content No longer needed after #77073. Made-with: Cursor * Breadcrumbs: Apply Text per item instead of wrapping nav Wrap each Link in Text with body-lg via nested render props (Text → Link → RouterLink) rather than using an outer Text on the nav element. Remove the h1 margin override. Made-with: Cursor * Breadcrumbs: Replace pseudo-element separator with explicit Text element Ensures the separator inherits the same typography styles as the adjacent link items by rendering it as a `<Text variant="body-lg">` instead of an `li::after` pseudo-element. Made-with: Cursor * Breadcrumbs: Prevent selection of decorative separator Made-with: Cursor * Update packages/admin-ui/src/breadcrumbs/index.tsx Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com> * Admin UI: Remove unused @wordpress/base-styles dependency Made-with: Cursor * Breadcrumbs: Prune now-unused ESLint suppressions * Breadcrumbs: Un-nest styles and fix separator class name CSS modules scope class names automatically, so nesting under .list was unnecessary. Also rename `.admin-ui-breadcrumbs__separator` to `.separator` to match the `styles.separator` lookup in the component, which was previously returning undefined. Made-with: Cursor * Breadcrumbs: Fix truncation and prevent wrapping in narrow containers Flex items default to `min-width: auto`, so the <li> wrapping the current item never shrinks below its content size and `text-overflow: ellipsis` on `.current` never engages. Add `min-width: 0` to the last <li> so the truncation styles can take effect. Also set `flex-shrink: 0` on preceding items so their text doesn't wrap to multiple lines when the container is narrow. Made-with: Cursor * Breadcrumbs: Derive min-height from --wpds-dimension-base Replace the hardcoded 32px with `calc(var(--wpds-dimension-base) * 8)` so the value tracks the design system's base unit. A @todo is left to swap this for a dedicated `size` token once that category exists in the theme package. Made-with: Cursor --------- Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: aduth <aduth@git.wordpress.org> Co-authored-by: simison <simison@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org>
What
Refactors the Breadcrumbs component to use
Link,Stack, andTextfrom@wordpress/uiinstead ofHStack,Heading, and the unstyled routerLink. Drops the@wordpress/base-stylesdependency from@wordpress/admin-uientirely (no other styles in the package needed it), introduces an explicit, accessible separator element, fixes truncation behavior, and tidies up the stylesheet for the CSS modules era.Why
The Breadcrumbs component was relying on experimental components from
@wordpress/components(__experimentalHStack,__experimentalHeading) and an unstyled router link. Migrating to@wordpress/uialigns breadcrumbs with the design system, giving links consistent interactive styling via theneutraltone and typography viaTextvariants. It also resolves a longstanding TODO in the stylesheet about adopting theTextcomponent, surfaces a couple of latent layout/styling bugs along the way, and removes a dependency that was no longer needed at the package level.How
Link(for styling) with the routerLink(for navigation) using therenderprop. Usestone="neutral"for breadcrumb-appropriate link styling.HStackwithStack direction="row", rendered as a<ul>via therenderprop.HeadingwithText variant="heading-lg", rendered as an<h1>via therenderprop. Applied per-item so each breadcrumb gets the correct typography rather than relying on a single wrapping element.Text aria-hiddenelement, uses a subdued--wpds-color-stroke-surface-neutralcolor, and disables text selection so the decorative/doesn't leak into copied selections..currentactually work — flex items default tomin-width: auto, so the wrapping<li>was never shrinking. Addedmin-width: 0on the last<li>so the ellipsis can engage, andflex-shrink: 0on preceding items so their labels don't wrap to multiple lines in narrow containers..currentand.separatorfrom.list(CSS modules already scope class names), renamed.admin-ui-breadcrumbs__separator→.separatorto match thestyles.separatorlookup in the component (the old name was returningundefined, leaving the separator unstyled), and replaced a hardcodedmin-height: 32pxwithcalc(var(--wpds-dimension-base) * 8)(with a@TODOto swap in asizetoken once that category exists).@wordpress/base-stylesfrompackages/admin-ui/package.json— nothing in the package imports it anymore.Linkto theuse-recommended-componentsallowlist for@wordpress/ui(with corresponding test updates), removes a now-unnecessaryheading-has-contentdisable, and prunes the now-unused suppressions entry.Before
After
Use of AI Tools
Created with Cursor & Claude Opus 4.7