Repository navigation
feat(ui): add Stepper component to @automattic/ui - #111036
Conversation
Jetpack Cloud Live (direct link)
Automattic for Agencies Live (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Jetpack Cloud Live (direct link)
Automattic for Agencies Live (direct link)
|
63b3db1 to
52654fe
Compare
|
Looks like one of the E2E tests has failed. You can fix them following these steps:
|
0deab44 to
9f9b8e1
Compare
8d2cbab to
4213efd
Compare
matticbot
added
the
[Status] Needs Review
| @@ -0,0 +1,77 @@ | |||
| // packages/ui/src/horizontal-stepper/style.module.scss | |||
| @use '@wordpress/base-styles/breakpoints'; | |||
| @use '@wordpress/base-styles/variables'; | |||
There was a problem hiding this comment.
I'd try to replace these all with @wordpress/theme style tokens.
There was a problem hiding this comment.
Breakpoints are required here because the container query threshold needs a static length value at parse time. CSS custom properties var(--wpds-*) are not valid in container query conditions and would be silently ignored by the browser.
A reasonable compromise would be hardcoding 599px (which is $break-small - 1) directly and dropping the @use dependency entirely.
What do you think?
| background: none; | ||
| border: none; | ||
| padding: variables.$grid-unit-15; | ||
| cursor: pointer; |
There was a problem hiding this comment.
You can use --wpds-cursor-control for pointer.
There was a problem hiding this comment.
Thanks for the suggestion, but the AI reviewer found an issue.
There's a known SPA-leak issue with this token in Calypso. Once @wordpress/theme/design-tokens.css is loaded anywhere in the session (which happens on several Reader pages), it sets --wpds-cursor-control: pointer on :root, and that value persists across page navigations. @wordpress/components Button picks it up globally and resolves to cursor: default even on pages that never imported the token directly. There's already a workaround in client/reader/user-profile/views/achievements/achievements-settings/style.scss with a comment documenting exactly this. Using the token in the stepper would spread that fragility further. Keeping cursor: pointer as a plain value is safer until the token scoping issue is resolved upstream.
There was a problem hiding this comment.
Is that just a hypothetical issue, or in practice, it's an issue if it leaks? Sounds like the component and UI button can use the same token (and value) without issues?
cc @mirka otherwise might be good material for prepping buttons to be stable.
There was a problem hiding this comment.
I haven't tested it, but found a comment in the codebase documenting exactly this issue: client/reader/user-profile/views/achievements/achievements-settings/style.scss has a cursor: pointer override with a note explaining that the token leaks across SPA navigations. So it appears to be a real, known issue.
There was a problem hiding this comment.
Thanks! It's also not a biggie to keep static so I'm mostly glad it surfaced the issue needing fixing. :-)
There was a problem hiding this comment.
it sets --wpds-cursor-control: pointer on :root, and that value persists across page navigations. @wordpress/components Button picks it up globally and resolves to cursor: default even on pages that never imported the token directly.
I'm confused to how Button would resolve to cursor: default if the theme styles set the token to --wpds-cursor-control: pointer ? (and the hardcoded fallback on Button is also pointer )?
From an initial look, I don't understand how this bug could happen.
I haven't tested it [...] it appears to be a real, known issue.
We should push back against the AI reviewer untested claim, and investigate further:
- If it's a real issue, let's open a dedicated GtiHub issue to track it, explain it, and follow-up to fix it (we definitely don't want to propagate hacks/style overrides across calypso)
- if it's not, we can undo the temporary fix in
client/reader/user-profile/views/achievements/achievements-settings/style.scssand use the token in this PR
Also fine to keep the current hardcoded pointer, but this follow-up work should happen sooner rather than later.
- Merge duplicated orientation guard in Stepper.List and move render-phase warning() calls into useEffect (Concurrent Mode safety) in List and Panel - Use imported ReactNode type in step.tsx for consistency - Replace `valueProp as string` cast with `valueProp ?? internalValue` - Export StepperRootProps from the package root as the PR description claims - Add missing coverage: horizontal Panel missing-value warning, VerticalStepper linear mode, and keepMounted passthrough Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The forwardRef wrappers already type and forward the ref; the explicit ref prop in the props type only forced an Omit workaround in StepperRootProps and leaked into the rest-prop spread type. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Thank you, @bogiii , for addressing most of the feedback that I left!
I understand that we'd prefer to merge this PR sooner rather than later.
I'm happy to pre-approve and allow this iteration, but we will definitely need to continue work on the component in follow-up PRs in the upcoming days. All of the unresolved comments in this PR will need to be worked on.
I also wanted to highlight the current state of the Stepper code when it comes to theming: it currently consumes WPDS tokens, which means that the component won't work as expected when enabling Calypso dark mode. After chatting with @aduth , we think that one of the most important follow-ups here should be to add a compat layer (extending client/lib/color-scheme/dark-theme.scss) to remap WPDS tokens to Calypso dark mode values, which will fix the issue at the right layer (calypso app).
Separately, we will likely need to revert the proprietary Dark Mode code, refactoring to @wordpress/theme — but that's unrelated from this PR.
| --a8c-ui-stepper-indicator-size: 18px; | ||
| --a8c-ui-stepper-indicator-size-sm: 16px; |
There was a problem hiding this comment.
Is it really necessary to have two different sizes for the indicator? Is this a sign that we should tweak the icons/layout to be able to use only one size in the CSS?
There was a problem hiding this comment.
I agree, but this follows the Figma design.
The dashed circle for upcoming steps is intentionally 2px wider than the solid circle used for current and completed steps. The margin: 1px compensates for the size difference so all indicator centers stay vertically aligned across states.
cc @nuriapenya
There was a problem hiding this comment.
We could keep the size fixed to 18px and account for these differences directly in the SVG?
| const halfCircleIcon = ( | ||
| <SVG | ||
| width="10" | ||
| height="10" | ||
| viewBox="0 0 10 10" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| aria-hidden="true" | ||
| style={ { transform: 'rotate(180deg)' } } | ||
| > | ||
| <Path d="M0 5a5 5 0 0 1 10 0H0z" fill="currentColor" /> | ||
| </SVG> | ||
| ); |
There was a problem hiding this comment.
Probably fine for this iteration, but I'd like to get feedback from @jameskoster @jasmussen @fcoveram to see if we can follow-up and only use standard @wordpress/icons icons
| if ( status === 'completed' ) { | ||
| indicator = <Icon icon={ check } size={ 14 } />; | ||
| } else if ( status === 'error' ) { | ||
| indicator = <span aria-hidden="true">!</span>; |
There was a problem hiding this comment.
Same here — implementing this custom solution could work as a temporary, initial solution, but I feel like in the long term we should uniform to a more DS-like implementation. cc @jasmussen @jameskoster @fcoveram
| &:hover:not( [aria-disabled='true'] ) { | ||
| .title { | ||
| color: var( --wpds-color-fg-interactive-brand ); | ||
| } | ||
|
|
||
| // Number-variant current step has a solid filled circle — skip border change | ||
| // to avoid an accent ring appearing against the dark background on hover. | ||
| .indicator:not( .is-current[data-indicator-variant='number'] ) { | ||
| border-color: var( --wpds-color-fg-interactive-brand ); | ||
| } | ||
|
|
||
| // Pull icon color for completed and current steps (SVG/dome use currentColor). | ||
| // Number-variant current steps keep their original number color on hover. | ||
| .indicator.is-completed, | ||
| .indicator.is-current:not( [data-indicator-variant='number'] ) { | ||
| color: var( --wpds-color-fg-interactive-brand ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Mixing and matching the tone (ie. neutral in the resting state, and brand in the hover state) should be avoided when using the tokens. The tokens themselves already offer an active variant that should be used for "active" states, such as hover/focus/active. The same applies for disabled states (there is a disabled variant of the interactive tokens).
Another mismatch that I noticed is the mixing of content and interactive tokens: we should use interactive tokens only for UI elements that are effectively interactive (such as links and buttons), and otherwise use content tokens.
In other words, we should use the tokens according to their semantic meaning — the resulting "look and feel" will automatically follow the WPDS aesthetic.
To summarize, we should pick one family of tokens for the interactive trigger / indicator / title, and avoid mixing and matching different families of DS tokens. We could probably go for fg-interactive-neutral{, -active, -disabled} and reserve fg-content-* for genuinely static, non interactive content.
There was a problem hiding this comment.
Agree on the content / interactive concern.
The neutral / brand point speaks to a system issue; --wpds-color-fg-interactive-neutral and --wpds-color-fg-interactive-neutral-active map to identical values, as tracked in WordPress/gutenberg#77153. So when backgrounds aren't utilised, switching foreground tone is one of very few reliable ways to achieve a visible hover style. Until we find a solution for this issue maybe it's okay to switch tone on hover? It shouldn't be harmful...?
There was a problem hiding this comment.
@jameskoster I'm a bit concerned about allowing mixing and matching tones, as it can easily get out of control and produce inconsistent behaviour across UIs.
As a thoguht exercise, I guess Stepper should behave essentially like @wordpress/ui Tabs or at most like a minimal neutral Button?
Of course, we can still discuss whether neutral UIs should use the brand color for their active state, but that information should be probably baked in the token, at not at the consumer's discretion?
There was a problem hiding this comment.
Update:
I spoke with @jameskoster and we agreed on the following:
- Theme: Some color token values that should differ do not WordPress/gutenberg#77153 is definitely an aspect that needs improving
- In any case, we shouldn't build UIs that rely only on one affordance (in this case, text color) to indicate a visual hover/focus state
We see two possible solutions:
- Use existing
@wordpress/uicomponents, in this caseButton, which already come with extra affordances (background color changes). This would likely need a light re-design to fitButtonbetter, but it would remove the need for custom markup/styles inStepper - Keep the custom UI + WPDS tokens, but add extra visual affordances for hover/focus states, eg making font weight bolder, slightly scale (animate) the icon
|
Thank you all for the reviews. I'll merge this PR to unlock shipping the experiment, but I'll continue handling and polishing this component in the follow-up PRs. |
github-actions
Bot
removed
the
[Status] Needs Review
|
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/33822996 Some locales (Hebrew, Japanese) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Thank you @bogiii for including a screenshot in the description! This is really helpful for our translators. |
Sounds good, @bogiii ! Is there a place to track the progress for all follow-up tasks? |
The duplicate-value warning in Stepper.Root was dead code: registration de-duped steps before the warning loop could ever see a duplicate. Move the warning into useStepRegistration where it can actually fire, and return a no-op deregister for rejected duplicates so unmounting one no longer removes the original step registration. Follow-up to #111036 review feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The union-keys comparison in updateStep was needed when registration meta carried ReactNode snapshots; meta is now flat scalars, so a plain shallow compare (with a key-count check for removed fields) is enough. Also drop children/className from StepperPanelProps since ComponentProps<'div'> already provides them. Follow-up to #111036 review feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
I'll handle the remaining feedback in small, easy-to-review follow-up PRs. For tracking, I'll reply under each open feedback thread in this PR with a link to the follow-up PR that addresses it, so each thread can be resolved once it's handled. How does it sound to you, @ciampo? Follow-up feedback checklist:
|
* ui: Remove dead Stepper title color and derive bullet margin from size tokens Follow-up to review feedback on #111036: - The base `.title` color never applied; it is always overridden by the higher-specificity `[data-indicator-variant] .title` rules since the root always renders `data-indicator-variant`. - Replace the magic `margin: 1px` on bullet indicators with a calc() derived from the indicator size variables, so the centre alignment stays correct if the sizes ever change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ui: Extract Stepper bullet offset into a shared custom property Replace the duplicated margin calc on current/completed bullet indicators with a single --a8c-ui-stepper-indicator-offset token. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…111567) * ui: Stepper warns about duplicate step values at registration time The duplicate-value warning in Stepper.Root was dead code: registration de-duped steps before the warning loop could ever see a duplicate. Move the warning into useStepRegistration where it can actually fire, and return a no-op deregister for rejected duplicates so unmounting one no longer removes the original step registration. Follow-up to #111036 review feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ui: Add changelog entry for Stepper duplicate-value warning fix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ui: Drop changelog entry for unreleased Stepper fix The Stepper component is still unreleased, so a bug fix to it needs no changelog entry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ui: Ref-count Stepper registrations so transient duplicates never drop a step A rejected duplicate previously got a no-op deregister, so if the originally-accepted instance unmounted while the duplicate stayed mounted, the value lost its registration entirely. Track a per-value mount count and remove the entry only when the last mounted instance deregisters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ui: Simplify Stepper updateStep compare and StepperPanelProps The union-keys comparison in updateStep was needed when registration meta carried ReactNode snapshots; meta is now flat scalars, so a plain shallow compare (with a key-count check for removed fields) is enough. Also drop children/className from StepperPanelProps since ComponentProps<'div'> already provides them. Follow-up to #111036 review feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address PR review feedback (#111567) - Document the stale-metadata limitation when duplicate step values overlap - Rename use-step-registration test to .ts (no JSX) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ui: Key Stepper registration by per-mount id to fix stale duplicate metadata Instead of keying steps by value, each Stepper.Step now registers under a unique useId. The public steps list is derived as one entry per value from the first still-mounted instance, so a duplicate value can no longer override or leave stale metadata on the surviving step. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ui: Pin Stepper step slot so a surviving duplicate keeps its position Order derived steps by each value's stable first-appearance slot instead of its current array position, so when an earlier duplicate instance unmounts the surviving instance keeps the value's original slot rather than jumping to the end. Adds tests for the slot invariant, the duplicate warning, and strengthens the surviving-metadata assertion to cover both mounted and unmounted states. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ui: Store Stepper slot in state so steps derivation stays pure Fold the per-value slot into the instances state and sort on it, so the steps memo no longer reads the slotOrder ref mid-render — keeping the derivation pure under concurrent rendering. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
|
Let's also not forget the |
9 participants
Part of DOTPROD-58
Proposed Changes
Steppercomponent family topackages/ui(@automattic/ui), built on@base-ui/reactv1.5Stepper.Root,Stepper.List,Stepper.Step,Stepper.Trigger,Stepper.Panel,Stepper.Indicator,Stepper.Title,Stepper.DescriptionVerticalStepper(accordion-style with animated expand/collapse) andHorizontalStepper(tab-style), both exported from the package rootStepper,VerticalStepper,HorizontalStepper, and types (StepperProps,StepperRootProps,StepperRef,StepProps,StepStatus,IndicatorVariant) from@automattic/uiWhy are these changes being made?
The design system lacked a Stepper component for guiding users through multi-step flows (onboarding, wizards, checkout sequences). This implementation provides both a flexible primitive layer for bespoke layouts and ready-to-use wrappers for the common vertical and horizontal patterns, all with full accessibility support (ARIA, keyboard navigation, screen reader labels) baked in.
Testing Instructions
VerticalStepperorHorizontalStepperfrom@automattic/uiand render a multi-step flowlinearprop restricts navigation to completed steps onlyformatStepLabelprop and the built-in visually-hidden indicator textyarn test-packages --testPathPattern="packages/ui/src/stepper"Pre-merge Checklist