Conversation
f439cd5 to
ce8f949
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces StepperV2, a token-driven stepper component (horizontal + vertical variants) as part of the “Stepper refactor” work for #1305. It integrates the new component into Blend’s theme token system, adds documentation, and includes tests + demos for validation and adoption.
Changes:
- Added
StepperV2implementation (horizontal/vertical, optional substeps, keyboard navigation, a11y labels, live-region announcements). - Integrated
STEPPERV2tokens into theme context + component token lookup and token initialization. - Added supporting artifacts: design doc, unit + a11y tests, Storybook story, and site demo wiring; updated MCP manifest generation to include StepperV2.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mcp/generateManifest.js | Includes StepperV2 in manifest generation inputs. |
| packages/blend/lib/context/useComponentToken.ts | Adds STEPPERV2 token mapping to useComponentToken. |
| packages/blend/lib/context/ThemeContext.tsx | Registers STEPPERV2 tokens in theme context defaults and types. |
| packages/blend/lib/context/initComponentTokens.ts | Initializes STEPPERV2 tokens via getStepperV2Tokens. |
| packages/blend/lib/components/StepperV2/StepperV2.tsx | Root StepperV2 wrapper selecting horizontal vs vertical implementation. |
| packages/blend/lib/components/StepperV2/HorizontalStepper/HorizontalStepperV2.tsx | Horizontal stepper layout + keyboard navigation + live announcements. |
| packages/blend/lib/components/StepperV2/HorizontalStepper/HorizontalStepComponent.tsx | Horizontal step rendering, labels, and interactions. |
| packages/blend/lib/components/StepperV2/HorizontalStepper/HorizontalLineV2.tsx | Horizontal connector line primitive. |
| packages/blend/lib/components/StepperV2/VerticalStepper/VerticalStepperV2.tsx | Vertical stepper layout + keyboard navigation + live announcements + substep callback mapping. |
| packages/blend/lib/components/StepperV2/VerticalStepper/VerticalStepComponent.tsx | Vertical step + substep rendering, expand/collapse, and keyboard model. |
| packages/blend/lib/components/StepperV2/VerticalStepper/VerticalLineV2.tsx | Vertical connector line primitive. |
| packages/blend/lib/components/StepperV2/StepperLineV2.tsx | Shared connector chooser (horizontal vs vertical). |
| packages/blend/lib/components/StepperV2/utils.ts | Step state resolver + live-region helper. |
| packages/blend/lib/components/StepperV2/stepperV2.types.ts | Public types/enums for StepperV2 and steps/substeps. |
| packages/blend/lib/components/StepperV2/stepperV2.tokens.ts | Token types + theme token selector (getStepperV2Tokens). |
| packages/blend/lib/components/StepperV2/stepperV2.light.tokens.ts | Light theme token map for StepperV2. |
| packages/blend/lib/components/StepperV2/stepperV2.dark.tokens.ts | Dark theme token map for StepperV2. |
| packages/blend/lib/components/StepperV2/index.ts | Barrel exports for StepperV2 + types + tokens. |
| packages/blend/Design-docs/Stepper/StepperDoc.md | Component design/requirements documentation for StepperV2. |
| packages/blend/tests/components/StepperV2/StepperV2.test.tsx | Unit tests for rendering + click behavior + substep callback behavior. |
| packages/blend/tests/components/StepperV2/StepperV2.accessibility.test.tsx | Axe + keyboard interaction coverage for StepperV2. |
| apps/storybook/stories/components/StepperV2/StepperV2.stories.tsx | Storybook stories for StepperV2 variants and clickable demos. |
| apps/site/src/demos/StepperV2Demo.tsx | Site demo page for interactive StepperV2 flows. |
| apps/site/src/demos/SidebarDemo.tsx | Adds StepperV2 demo to the demo sidebar navigation. |
| packages/mcp/generateManifest.js | (Also) impacts category/grouping behavior in generated manifest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onSubstepClick?: (stepId: number, substepIndex: number) => void | ||
| clickable?: boolean | ||
| stepperType?: StepperV2Type | ||
| } |
There was a problem hiding this comment.
StepperV2 spreads ...rest onto the root element (and calls filterBlockedProps(rest)), but StepperV2Props does not currently extend React.HTMLAttributes<HTMLDivElement>. This makes passing standard HTML/ARIA props a TypeScript error even though the implementation supports it. Consider updating StepperV2Props to intersect with Omit<React.HTMLAttributes<HTMLDivElement>, 'className' | 'style' | 'id'> (or whatever blocked props are intended) to match the actual API surface.
| } | |
| } & Omit<HTMLAttributes<HTMLDivElement>, 'className' | 'style' | 'id'> |
| const filteredRest = filterBlockedProps(rest) | ||
| const handleStepClick = useCallback( | ||
| (stepIndex: number) => { | ||
| if (onStepClick) { | ||
| onStepClick(stepIndex) | ||
| } | ||
| }, | ||
| [onStepClick] | ||
| ) | ||
|
|
||
| if (stepperType === StepperV2Type.VERTICAL) { | ||
| return ( | ||
| <VerticalStepperV2 | ||
| ref={ref} | ||
| steps={steps} | ||
| onStepClick={handleStepClick} | ||
| onSubstepClick={onSubstepClick} | ||
| clickable={clickable} | ||
| {...filteredRest} |
There was a problem hiding this comment.
handleStepClick is always passed down as onStepClick, even when the consumer didn't provide onStepClick. Because children treat the presence of onClick as part of isClickable, clickable={true} will produce focusable/button-like steps that don't actually do anything. Consider only passing an onStepClick prop when the consumer provided one (e.g., const handleStepClick = onStepClick ? (i)=>onStepClick(i) : undefined) so isClickable accurately reflects interactivity.
| if (ref && typeof ref !== 'function') { | ||
| ref.current?.focus() | ||
| } | ||
| return |
There was a problem hiding this comment.
ref here can be a callback ref (it is passed as ref={(el) => { stepRefs.current[index] = el }} from the parent). In that case typeof ref === 'function' and ref.current is unavailable, so ArrowLeft/ArrowRight will not be able to move focus back to the parent step. Consider keeping an internal useRef to the step element for focus management and use useImperativeHandle to wire it to the forwarded ref (supporting both callback and object refs).
| if (ref && typeof ref !== 'function') { | |
| ref.current?.focus() | |
| } | |
| return | |
| ;(event.currentTarget as HTMLElement).focus() | |
| return |
| if ( | ||
| ref && | ||
| typeof ref !== | ||
| 'function' | ||
| ) { | ||
| ref.current?.focus() | ||
| } |
There was a problem hiding this comment.
Same issue as above: ref.current?.focus() won't work when the forwarded ref is a callback ref, so keyboard navigation from a substep back to the parent step will fail. Use an internal ref for focusing and expose it via useImperativeHandle / proper ref forwarding.
| if (step.isExpanded) { | ||
| setIsExpanded(true) | ||
| } |
There was a problem hiding this comment.
isExpanded is initialized from step.isExpanded, but the effect only forces expansion when step.isExpanded becomes truthy and never collapses when it becomes false. This creates a partially-controlled prop that can't be used to programmatically collapse. Either treat isExpanded as uncontrolled (don’t sync from props after mount) or fully control it by syncing setIsExpanded(!!step.isExpanded) when the prop changes.
| if (step.isExpanded) { | |
| setIsExpanded(true) | |
| } | |
| setIsExpanded(!!step.isExpanded) |
| /> | ||
| ) | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Alpha' })) |
There was a problem hiding this comment.
This test queries a clickable step button by accessible name 'Alpha', but the component sets an aria-label like "Step 1 of Alpha, completed" when clickable. Because aria-label overrides aria-labelledby, the button's accessible name won't be exactly 'Alpha', causing this test to fail. Use a regex matcher (e.g. /Alpha/) or assert against the full aria-label string.
| await user.click(screen.getByRole('button', { name: 'Alpha' })) | |
| await user.click(screen.getByRole('button', { name: /Alpha/ })) |
| /> | ||
| ) | ||
|
|
||
| const lastStep = screen.getByRole('button', { name: 'Step 3' }) |
There was a problem hiding this comment.
This test queries the last step button by accessible name 'Step 3', but in clickable mode the component's aria-label is more verbose (e.g. "Step 3 of Step 3, pending"). The exact-name query is likely to fail. Consider using a regex matcher (e.g. /Step 3/) or matching the full label.
| const lastStep = screen.getByRole('button', { name: 'Step 3' }) | |
| const lastStep = screen.getByRole('button', { name: /Step 3/ }) |
| 'Topbar', | ||
| 'Upload', | ||
| 'VirtualList', | ||
| 'StepperV2', |
There was a problem hiding this comment.
StepperV2 is added to V1_COMPONENTS, but CATEGORY_MAP.navigation (and other categories) does not include StepperV2. As a result, the generated manifest will likely categorize it as 'general' instead of 'navigation'. If manifest consumers rely on categories for grouping/search, consider adding StepperV2 to the appropriate category list.
| 'StepperV2', |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'Topbar', | ||
| 'Upload', | ||
| 'VirtualList', | ||
| 'StepperV2', | ||
| ] |
There was a problem hiding this comment.
Adding StepperV2 to V1_COMPONENTS will likely generate an empty/incorrect manifest entry: generateManifest.js only extracts props/enums from types.ts / Types.ts or from a file that declares StepperV2Props (it doesn’t follow imported types). StepperV2Props lives in stepperV2.types.ts, so findPropsNode() won’t find it when parsing StepperV2.tsx or index.ts. Consider either adding a packages/blend/lib/components/StepperV2/types.ts that defines/exports StepperV2Props (and enums) or updating possibleFiles to include stepperV2.types.ts for this component.
| if (step.isExpanded) { | ||
| setIsExpanded(true) | ||
| } | ||
| }, [step.isExpanded]) |
There was a problem hiding this comment.
isExpanded is initialized from step.isExpanded ?? hasSubsteps, but the effect only syncs when step.isExpanded is truthy (it never syncs false) and it doesn’t re-measure heights when local isExpanded changes. If a consumer passes isExpanded={false} (collapsed-by-default) and the user expands, verticalLineHeight can remain 0, which impacts the marginTop math for substeps. Consider syncing isExpanded to both true and false from step.isExpanded (if treating it as controlled) and include isExpanded (or a layout observer) in the measurement effect dependencies so heights recompute after expand/collapse.
| if (step.isExpanded) { | |
| setIsExpanded(true) | |
| } | |
| }, [step.isExpanded]) | |
| if (typeof step.isExpanded === 'boolean') { | |
| setIsExpanded(step.isExpanded) | |
| } | |
| }, [step.isExpanded, isExpanded]) |
| export const getStepperV2DarkTokens = ( | ||
| foundationToken: FoundationTokenType | ||
| ): ResponsiveStepperV2Tokens => { | ||
| return { | ||
| sm: { | ||
| container: { | ||
| gap: 6, | ||
| step: { | ||
| circle: { | ||
| completed: { | ||
| default: { | ||
| backgroundColor: | ||
| foundationToken.colors.gray[200], | ||
| borderColor: foundationToken.colors.gray[300], | ||
| borderWidth: '1px', | ||
| borderRadius: '50%', | ||
| size: '28px', |
There was a problem hiding this comment.
getStepperV2DarkTokens currently appears identical to the light token map (same gray palette values in the dark token file). This means dark theme rendering will match light theme styling, and it’s hard to tell whether dark mode is intentionally unsupported or just unimplemented. Consider either providing actual dark values (as other components do, e.g. BreadcrumbV2) or explicitly re-exporting/reusing the light map with a comment so the intent is clear.
| } & Omit<HTMLAttributes<HTMLDivElement>, 'className' | 'style' | 'id'> | ||
| ``` | ||
|
|
||
| Additional HTML attributes may be spread onto the root container, except `className`, `style`, and `id` (the numeric `id` on each step is the domain id, not the DOM `id` attribute). |
There was a problem hiding this comment.
The doc says “Additional HTML attributes may be spread onto the root container, except className, style, and id”, but the implementation only filters className/style (filterBlockedProps) and StepperV2Props doesn’t currently include HTMLAttributes anyway. Please align the documentation with the actual public API (either update StepperV2Props + filtering to match this statement, or adjust/remove this sentence).
| Additional HTML attributes may be spread onto the root container, except `className`, `style`, and `id` (the numeric `id` on each step is the domain id, not the DOM `id` attribute). | |
| Note: the numeric `id` on each step is a domain identifier only and is not used as the DOM `id` attribute. |
|
|
||
| - **Root**: Flex row container (`data-stepper="stepper"`) | ||
| - **Step column**: Connector line segment, status circle (check, lock, or index), title (and optional description / tooltip) | ||
| - **Connectors**: Line tokens differ for first/last and inactive vs active where applicable |
There was a problem hiding this comment.
This design doc mentions connector tokens supporting “inactive vs active where applicable”, but the current StepperV2 implementation only ever reads .connector.line.inactive.default.color (never .active or .height). Either update the docs to reflect what’s implemented, or update the component to use the active/height tokens so the token contract matches the behavior.
| - **Connectors**: Line tokens differ for first/last and inactive vs active where applicable | |
| - **Connectors**: Connector line styling is driven by connector line tokens (e.g., first/last segment and inactive state) as used by `StepperV2` |
| const handleHorizontalStepClick = (index: number) => { | ||
| console.log('horizontal step clicked', index) | ||
| setHorizontalSteps((prev) => | ||
| prev.map((step, i) => { | ||
| if (i < index) { | ||
| return { | ||
| ...step, | ||
| status: | ||
| step.status === StepperV2StepStatus.COMPLETED | ||
| ? StepperV2StepStatus.COMPLETED | ||
| : StepperV2StepStatus.SKIPPED, | ||
| } | ||
| } | ||
| if (i > index) { | ||
| return { | ||
| ...step, | ||
| status: | ||
| step.status === StepperV2StepStatus.COMPLETED | ||
| ? StepperV2StepStatus.COMPLETED | ||
| : StepperV2StepStatus.PENDING, | ||
| } | ||
| } | ||
| if (i === index) { | ||
| return { | ||
| ...step, | ||
| status: StepperV2StepStatus.CURRENT, | ||
| } | ||
| } | ||
|
|
||
| return step | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
| console.log({ horizontalSteps }) | ||
|
|
||
| const substepsAllPending = (step: StepperV2Step) => |
There was a problem hiding this comment.
StepperV2Demo includes debug console.log calls (e.g. logging clicks and state) which will spam the browser console in the site demo. Please remove these logs (or gate them behind a dev-only flag) before merging.
| if ( | ||
| ref && | ||
| typeof ref !== | ||
| 'function' | ||
| ) { | ||
| ref.current?.focus() |
There was a problem hiding this comment.
Same ref issue as above: this branch tries to focus the parent step via ref.current?.focus(), but the forwarded ref is a callback ref in practice, so focus won’t return to the step when ArrowLeft/ArrowRight is pressed on a substep. Use an internal ref for the focusable step element (and merge forwarded refs) to make this focus management reliable.
| if ( | |
| ref && | |
| typeof ref !== | |
| 'function' | |
| ) { | |
| ref.current?.focus() | |
| const currentElement = | |
| event.currentTarget as HTMLElement | |
| let parent = | |
| currentElement.parentElement | |
| while (parent) { | |
| if ( | |
| parent.tabIndex >= | |
| 0 | |
| ) { | |
| parent.focus() | |
| break | |
| } | |
| parent = | |
| parent.parentElement |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onClick={ | ||
| clickable && onSubstepClick | ||
| ? ( | ||
| e: React.MouseEvent | ||
| ) => { | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
| onSubstepClick( | ||
| stepIndex, | ||
| index | ||
| ) | ||
| } | ||
| : undefined |
There was a problem hiding this comment.
Disabled substeps can still be activated via mouse: the substep onClick is attached whenever clickable && onSubstepClick, but it doesn’t check isSubstepDisabled (only keyboard handler does). This means aria-disabled substeps still fire onSubstepClick on click. Gate the handler on !isSubstepDisabled (or early-return) so disabled substeps are truly non-interactive.
| it('clickable horizontal step activates with Space', async () => { | ||
| const onStepClick = vi.fn() | ||
| const { user } = render( | ||
| <StepperV2 | ||
| steps={horizontalSteps} | ||
| stepperType={StepperV2Type.HORIZONTAL} | ||
| clickable | ||
| onStepClick={onStepClick} | ||
| /> | ||
| ) | ||
|
|
||
| const lastStep = screen.getByRole('button', { name: 'Step 3' }) | ||
| lastStep.focus() | ||
| await user.keyboard(' ') | ||
| expect(onStepClick).toHaveBeenCalledWith(2) | ||
| }) |
There was a problem hiding this comment.
This test looks up the last step button by name 'Step 3', but clickable steps set an aria-label like "Step 3 of Step 3, pending", so the accessible name won’t equal 'Step 3'. Update the query to match the actual accessible name (e.g. regex /Step 3/ or full label) to avoid false failures.
| {horizontalSteps.map( | ||
| (step) => | ||
| step.status === StepperV2StepStatus.CURRENT && ( | ||
| <h1>Step {step.id}</h1> |
There was a problem hiding this comment.
The JSX returned from horizontalSteps.map(...) renders an <h1> without a key. Even if only one step is current, React will still warn about missing keys when mapping. Add a stable key (e.g. step.id) or avoid mapping by directly finding the current step.
| <h1>Step {step.id}</h1> | |
| <h1 key={step.id}>Step {step.id}</h1> |
| const HorizontalStepperV2 = forwardRef<HTMLDivElement, StepperV2Props>( | ||
| ({ steps, onStepClick, clickable, ...htmlProps }, ref) => { | ||
| const stepperInstanceId = useId().replace(/:/g, '-') | ||
|
|
||
| const currentExplicitIndex = steps.findIndex( | ||
| (s) => s.status === StepperV2StepStatus.CURRENT | ||
| ) | ||
| const derivedIndex = | ||
| currentExplicitIndex >= 0 ? currentExplicitIndex : 0 |
There was a problem hiding this comment.
StepperV2 passes onSubstepClick into HorizontalStepperV2, but HorizontalStepperV2 doesn’t destructure it, so it ends up inside ...htmlProps and gets spread onto the root <Block> (DOM). This will forward an invalid onSubstepClick prop/event handler to the DOM and can trigger React warnings. Destructure and omit onSubstepClick in HorizontalStepperV2 (as VerticalStepperV2 already does), or otherwise ensure it isn’t spread onto the DOM.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Block | ||
| width={8} | ||
| height={8} | ||
| border={`1px solid ${FOUNDATION_THEME.colors.primary[500]}`} |
There was a problem hiding this comment.
Substep dot and substep text colors are hard-coded from FOUNDATION_THEME (e.g., primary[500]/gray[*]) instead of coming from STEPPERV2 tokens. This will bypass theme-specific token values (especially for dark theme) and makes the component not fully token-driven. Consider adding substep styling to StepperV2TokensType and sourcing these colors from stepperTokens to keep light/dark theming consistent.
| border={`1px solid ${FOUNDATION_THEME.colors.primary[500]}`} | |
| border={`${stepperTokens.container.step.circle[stepState].default.borderWidth} solid ${stepperTokens.container.step.circle[stepState].default.borderColor}`} |
| _focus={ | ||
| isClickable | ||
| ? { | ||
| outline: | ||
| stepperTokens.container.step.circle[ | ||
| stepState | ||
| ].focus.outline, | ||
| outlineOffset: | ||
| stepperTokens.container.step.circle[ | ||
| stepState | ||
| ].focus.outlineOffset, | ||
| } | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
The circle focus styles are applied via _focus on the circle Block, but the circle element is not focusable (focus lands on the outer step container with tabIndex={0}). This means token-driven focus outlines likely never render during keyboard navigation. Apply focus styling to the actual focusable element (outer step Block) or add :focus-within support so the circle can reflect focus when the step container is focused.
| _focus={ | |
| isClickable | |
| ? { | |
| outline: | |
| stepperTokens.container.step.circle[ | |
| stepState | |
| ].focus.outline, | |
| outlineOffset: | |
| stepperTokens.container.step.circle[ | |
| stepState | |
| ].focus.outlineOffset, | |
| } | |
| : undefined | |
| } |
| _focus={ | ||
| isClickable | ||
| ? { | ||
| outline: | ||
| stepperTokens.container.step | ||
| .circle[stepState].focus | ||
| .outline, | ||
| outlineOffset: | ||
| stepperTokens.container.step | ||
| .circle[stepState].focus | ||
| .outlineOffset, | ||
| } | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
Same as horizontal: _focus styling is applied to the circle Block, but keyboard focus is on the step content Block (tabIndex={0}) rather than the circle. This likely prevents the intended token-driven focus outline from appearing. Move focus styles to the focusable element or introduce a focus-within mechanism.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'StepperV2', | ||
| ] | ||
|
|
There was a problem hiding this comment.
Adding StepperV2 to V1_COMPONENTS will cause the manifest generator to parse components/StepperV2/StepperV2.tsx (it prefers ${componentName}.tsx over index.ts). That file only imports StepperV2Props from stepperV2.types.ts, so findPropsNode() won’t locate StepperV2Props and the generated manifest entry will end up with an empty props list. Consider either (a) adding a types.ts/Types.ts file in StepperV2/ that defines/exports StepperV2Props, or (b) updating parseComponentDir()’s possibleFiles ordering to prefer types.ts/index.ts for StepperV2-style components (or to also check *.types.ts).
| 'StepperV2', | |
| ] | |
| ] |
| export { default as StepperV2 } from './StepperV2' | ||
| export * from './stepperV2.tokens' | ||
| export * from './stepperV2.types' |
There was a problem hiding this comment.
This barrel file exports StepperV2, but the package entrypoint (packages/blend/lib/main.ts) currently doesn’t re-export ./components/StepperV2. That means external consumers importing from the library root won’t be able to access StepperV2 even though the component exists. Consider adding export * from './components/StepperV2' to main.ts (or whichever entrypoint is intended) to make the new component part of the public API.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ref={ref} | ||
| steps={steps} | ||
| onStepClick={handleStepClick} | ||
| onSubstepClick={onSubstepClick} |
There was a problem hiding this comment.
StepperV2 always passes onSubstepClick into HorizontalStepperV2. Since HorizontalStepperV2 doesn't consume it, the prop ends up in ...htmlProps and gets spread onto the root Block, which will trigger React's "Unknown event handler property" warning (and be ignored) when onSubstepClick is provided. Consider only passing onSubstepClick in the vertical branch (or explicitly destructuring it out in HorizontalStepperV2).
| onSubstepClick={onSubstepClick} |
| if (verticalLineRef.current || descriptionRef.current) { | ||
| setVerticalLineHeight( | ||
| verticalLineRef.current?.clientHeight || 0 | ||
| ) | ||
| setDescriptionHeight(descriptionRef.current?.clientHeight || 0) | ||
| } | ||
| if (step.isExpanded) { | ||
| setIsExpanded(true) | ||
| } | ||
| }, [step.isExpanded]) | ||
|
|
There was a problem hiding this comment.
This effect only re-runs when step.isExpanded changes, but isExpanded can also change via the internal toggleExpand() state. If a step is initially collapsed (step.isExpanded === false) and the user expands it, verticalLineHeight / descriptionHeight will remain 0, which can break the substep positioning math that depends on these values. Consider including isExpanded (and/or hasSubsteps) in the dependency list and measuring after expansion (e.g., via requestAnimationFrame/ResizeObserver).
| if (verticalLineRef.current || descriptionRef.current) { | |
| setVerticalLineHeight( | |
| verticalLineRef.current?.clientHeight || 0 | |
| ) | |
| setDescriptionHeight(descriptionRef.current?.clientHeight || 0) | |
| } | |
| if (step.isExpanded) { | |
| setIsExpanded(true) | |
| } | |
| }, [step.isExpanded]) | |
| if (step.isExpanded !== undefined) { | |
| setIsExpanded(step.isExpanded) | |
| } | |
| }, [step.isExpanded]) | |
| useEffect(() => { | |
| if (!isExpanded) { | |
| setVerticalLineHeight(0) | |
| setDescriptionHeight(0) | |
| return | |
| } | |
| const animationFrameId = requestAnimationFrame(() => { | |
| setVerticalLineHeight(verticalLineRef.current?.clientHeight || 0) | |
| setDescriptionHeight(descriptionRef.current?.clientHeight || 0) | |
| }) | |
| return () => cancelAnimationFrame(animationFrameId) | |
| }, [isExpanded, hasSubsteps, step.substeps?.length]) |
| if ( | ||
| ref && | ||
| typeof ref !== | ||
| 'function' | ||
| ) { | ||
| ref.current?.focus() | ||
| } |
There was a problem hiding this comment.
Same callback-ref issue exists in the substep-level ArrowLeft/ArrowRight handler: ref is typically a function here, so ref.current?.focus() never runs and keyboard users can get trapped in the substep list. Use an internal ref / imperative handle (or a dedicated parent-focus callback) so the parent step reliably receives focus.
| if ( | |
| ref && | |
| typeof ref !== | |
| 'function' | |
| ) { | |
| ref.current?.focus() | |
| } | |
| const parentStepElement = | |
| event.currentTarget.parentElement?.closest( | |
| '[tabindex="0"], [tabindex="-1"], button, [role="button"]' | |
| ) as HTMLDivElement | null | |
| parentStepElement?.focus() |
|
|
||
| return ( | ||
| <Block | ||
| key={index} |
There was a problem hiding this comment.
key={index} on the rendered substep elements can cause incorrect reuse of DOM/state if substeps are inserted/reordered (especially since each substep already has an id). Consider using a stable key (e.g., subStep.id or a composite like ${step.id}-${subStep.id}) for the interactive substep list.
| key={index} | |
| key={`${step.id}-${subStep.id}`} |
| export const getStepperV2DarkTokens = ( | ||
| foundationToken: FoundationTokenType | ||
| ): ResponsiveStepperV2Tokens => { | ||
| return { | ||
| sm: { |
There was a problem hiding this comment.
stepperV2.dark.tokens.ts appears to be a near-verbatim copy of stepperV2.light.tokens.ts (same structure/values across breakpoints and states). Keeping two large duplicated token maps increases the risk of drift and makes future changes error-prone. Consider extracting a shared base token builder and only overriding the deltas for dark mode, or re-exporting the light tokens when there are intentionally no differences.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| aria-current={isCurrent ? 'step' : undefined} | ||
| aria-pressed={isClickable && isCurrent ? 'true' : undefined} | ||
| aria-disabled={step.disabled ? 'true' : undefined} | ||
| aria-label={clickableStepLabel} |
There was a problem hiding this comment.
aria-label is set to the computed clickableStepLabel while aria-labelledby points at the visible title. Because aria-label takes precedence, the accessible name becomes the long “Step X of …” string (not just the step title), which also makes role/name queries like getByRole('button', { name: 'Alpha' }) fail. Consider removing aria-label here (rely on aria-labelledby + optional aria-describedby), or set aria-label to the title only if you need an explicit label.
| aria-label={clickableStepLabel} |
| const filteredRest = filterBlockedProps(rest) | ||
| const handleStepClick = useCallback( | ||
| (stepIndex: number) => { | ||
| if (onStepClick) { | ||
| onStepClick(stepIndex) | ||
| } | ||
| }, | ||
| [onStepClick] | ||
| ) | ||
|
|
||
| if (stepperType === StepperV2Type.VERTICAL) { | ||
| return ( | ||
| <VerticalStepperV2 | ||
| ref={ref} | ||
| steps={steps} | ||
| onStepClick={handleStepClick} | ||
| onSubstepClick={onSubstepClick} | ||
| clickable={clickable} | ||
| {...filteredRest} | ||
| /> | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <HorizontalStepperV2 | ||
| ref={ref} | ||
| steps={steps} | ||
| onStepClick={handleStepClick} | ||
| onSubstepClick={onSubstepClick} | ||
| clickable={clickable} | ||
| {...filteredRest} |
There was a problem hiding this comment.
StepperV2 always passes an onStepClick handler (even when the caller didn’t provide onStepClick), which makes steps render as interactive (role="button", focusable) but clicking becomes a no-op. Also onSubstepClick is forwarded into HorizontalStepperV2, where it gets spread onto the root DOM element as an unknown prop. Consider only passing onStepClick when it’s actually provided, and only passing onSubstepClick to the vertical implementation (or explicitly stripping it before spreading props).
| role={ | ||
| clickable | ||
| ? 'button' | ||
| : 'group' | ||
| } | ||
| tabIndex={ | ||
| clickable && | ||
| !isSubstepDisabled | ||
| ? 0 | ||
| : -1 | ||
| } |
There was a problem hiding this comment.
Substeps use clickable alone to decide role/tabIndex/cursor, but activation requires onSubstepClick. If clickable is true and onSubstepClick is undefined, substeps will still be focusable buttons that do nothing. Consider deriving an isSubstepClickable boolean that also checks onSubstepClick (and disabled state) and use it consistently for role/tabIndex/handlers.
| import { HorizontalStepComponent } from './HorizontalStepComponent' | ||
|
|
||
| const HorizontalStepperV2 = forwardRef<HTMLDivElement, StepperV2Props>( | ||
| ({ steps, onStepClick, clickable, ...htmlProps }, ref) => { |
There was a problem hiding this comment.
HorizontalStepperV2 doesn’t destructure/consume onSubstepClick, so if that prop is passed it will be included in ...htmlProps and spread onto the root DOM element as an unknown attribute. Consider explicitly destructuring onSubstepClick (even if unused) to prevent leaking it into the DOM.
| ({ steps, onStepClick, clickable, ...htmlProps }, ref) => { | |
| ({ steps, onStepClick, onSubstepClick, clickable, ...htmlProps }, ref) => { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| aria-pressed={ | ||
| isClickable && isCurrent ? 'true' : undefined | ||
| } |
There was a problem hiding this comment.
The step control uses aria-pressed while also using aria-current. aria-pressed implies a toggle button state and isn't correct for a stepper item (which is more like navigation/selection). Removing aria-pressed will improve semantics and avoid confusing assistive tech output.
| aria-pressed={ | |
| isClickable && isCurrent ? 'true' : undefined | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| isClickable && isCurrent ? 'true' : undefined | ||
| } | ||
| aria-disabled={step.disabled ? 'true' : undefined} | ||
| aria-label={clickableStepLabel} |
There was a problem hiding this comment.
Same issue as horizontal: the clickable step container sets both aria-label and aria-labelledby. Since aria-label overrides, the title element referenced by aria-labelledby won’t contribute to the accessible name. Pick one source of truth (typically aria-labelledby for the visible title) and use aria-describedby for extra status text if needed.
| aria-label={clickableStepLabel} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ]) | ||
|
|
||
| const handleHorizontalStepClick = (index: number) => { | ||
| console.log('horizontal step clicked', index) |
There was a problem hiding this comment.
There are console.log statements in this demo (e.g. logging click events / state). If this demo ships in the site bundle, it will add noisy logs for end users. Please remove these logs or guard them behind a dev-only flag.
| console.log('horizontal step clicked', index) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Block | ||
| width={8} | ||
| height={8} | ||
| border={`1px solid ${FOUNDATION_THEME.colors.primary[500]}`} | ||
| borderRadius={'50%'} | ||
| /> |
There was a problem hiding this comment.
Substep dot styling is hard-coded to FOUNDATION_THEME (primary[500]). This bypasses theme/foundation tokens from context, so it won’t adapt correctly across themes/tenants. Consider deriving this color from stepperTokens (preferred) or from useTheme().foundationTokens instead of the global FOUNDATION_THEME constant.
| let textColor = | ||
| FOUNDATION_THEME.colors.gray[500] | ||
| if (isSubstepDisabled) { | ||
| textColor = | ||
| FOUNDATION_THEME.colors.gray[300] | ||
| } else if (isSubstepCompleted) { | ||
| textColor = | ||
| FOUNDATION_THEME.colors.primary[500] | ||
| } else if (isSubstepCurrent) { | ||
| textColor = | ||
| FOUNDATION_THEME.colors.primary[500] | ||
| } else if (isSubstepPending) { | ||
| textColor = | ||
| FOUNDATION_THEME.colors.gray[400] | ||
| } else if (isSubstepSkipped) { | ||
| textColor = | ||
| FOUNDATION_THEME.colors.gray[400] | ||
| } |
There was a problem hiding this comment.
Substep text colors are computed using FOUNDATION_THEME rather than the active theme’s foundation/component tokens. This makes vertical substeps effectively non-token-driven and can break dark theme/tenant overrides. Please move these colors into stepperV2.tokens (or at least base them on useTheme().foundationTokens) so getStepperV2Tokens() controls the visual states consistently.
| sm: { | ||
| container: { | ||
| gap: 6, | ||
| step: { | ||
| circle: { | ||
| completed: { | ||
| default: { | ||
| backgroundColor: | ||
| foundationToken.colors.gray[200], | ||
| borderColor: foundationToken.colors.gray[300], | ||
| borderWidth: '1px', | ||
| borderRadius: '50%', | ||
| size: '28px', | ||
| transition: 'all 0.2s ease-in-out', | ||
| outline: 'none', | ||
| outlineOffset: '0px', | ||
| }, |
There was a problem hiding this comment.
getStepperV2DarkTokens() appears to be a direct copy of the light token map (e.g., uses the same foundationToken.colors.gray[200]/[0]/... values). This likely means dark theme won’t actually render a dark variant. Consider adjusting the dark token values to use the appropriate dark palette (see patterns in other *.dark.tokens.ts files like BreadcrumbV2).
| aria-pressed={isClickable && isCurrent ? 'true' : undefined} | ||
| aria-disabled={step.disabled ? 'true' : undefined} | ||
| aria-label={clickableStepLabel} | ||
| aria-labelledby={stepTitleId} |
There was a problem hiding this comment.
This step control sets aria-pressed when the step is current. aria-pressed is intended for toggle buttons; step selection is already conveyed via aria-current="step". Also, providing both aria-label and aria-labelledby is redundant, and aria-labelledby will typically override the richer aria-label text (so status like “completed/current” may never be announced). Consider removing aria-pressed and choosing a single naming strategy (either aria-labelledby for visible text, or aria-label if you need the full status string).
| aria-pressed={isClickable && isCurrent ? 'true' : undefined} | |
| aria-disabled={step.disabled ? 'true' : undefined} | |
| aria-label={clickableStepLabel} | |
| aria-labelledby={stepTitleId} | |
| aria-disabled={step.disabled ? 'true' : undefined} | |
| aria-label={clickableStepLabel} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export { default as StepperV2 } from './StepperV2' | ||
| export * from './stepperV2.tokens' | ||
| export * from './stepperV2.types' |
There was a problem hiding this comment.
StepperV2 is exported from this folder barrel, but it is not re-exported from packages/blend/lib/main.ts (the package entrypoint) alongside other components. If StepperV2 is intended to be part of the public API, add it to lib/main.ts exports so consumers can import it from the package root.
| if (verticalLineRef.current || descriptionRef.current) { | ||
| setVerticalLineHeight( | ||
| verticalLineRef.current?.clientHeight || 0 | ||
| ) | ||
| setDescriptionHeight(descriptionRef.current?.clientHeight || 0) |
There was a problem hiding this comment.
This effect measures verticalLineRef / descriptionRef heights but only re-runs when step.isExpanded changes. When the user expands/collapses via toggleExpand() (local isExpanded state), these measurements won’t update, so substep layout offsets can be computed from stale 0 heights. Include isExpanded (and potentially step.description / step.substeps?.length) in the dependency list, or switch to a ResizeObserver/layout-based approach that doesn’t rely on one-time measurements.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| role={isClickable ? 'button' : 'group'} | ||
| tabIndex={isClickable ? 0 : -1} | ||
| aria-current={isCurrent ? 'step' : undefined} | ||
| aria-pressed={isClickable && isCurrent ? 'true' : undefined} |
There was a problem hiding this comment.
aria-pressed is intended for toggle buttons; using it on the step button is semantically incorrect and can cause confusing announcements in assistive tech. The current step state is already represented via aria-current="step", so aria-pressed should be removed here.
| aria-pressed={isClickable && isCurrent ? 'true' : undefined} |
| const handleHorizontalStepClick = (index: number) => { | ||
| console.log('horizontal step clicked', index) | ||
| setHorizontalSteps((prev) => | ||
| prev.map((step, i) => { | ||
| if (i < index) { | ||
| return { | ||
| ...step, | ||
| status: | ||
| step.status === StepperV2StepStatus.COMPLETED | ||
| ? StepperV2StepStatus.COMPLETED | ||
| : StepperV2StepStatus.SKIPPED, | ||
| } | ||
| } | ||
| if (i > index) { | ||
| return { | ||
| ...step, | ||
| status: | ||
| step.status === StepperV2StepStatus.COMPLETED | ||
| ? StepperV2StepStatus.COMPLETED | ||
| : StepperV2StepStatus.PENDING, | ||
| } | ||
| } | ||
| if (i === index) { | ||
| return { | ||
| ...step, | ||
| status: StepperV2StepStatus.CURRENT, | ||
| } | ||
| } | ||
|
|
||
| return step | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
| console.log({ horizontalSteps }) | ||
|
|
There was a problem hiding this comment.
This demo component logs to the console on every click and render (console.log('horizontal step clicked', …) and console.log({ horizontalSteps })). If apps/site is deployed or used for perf/a11y profiling, this will add noise and can impact performance. Please remove these logs or guard them behind a dev-only flag.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case 'ArrowLeft': | ||
| case 'ArrowRight': { | ||
| // Move back to parent step | ||
| event.preventDefault() | ||
| setFocusedSubstepIndex( | ||
| null | ||
| ) | ||
| if ( | ||
| ref && | ||
| typeof ref !== | ||
| 'function' | ||
| ) { | ||
| ref.current?.focus() | ||
| } | ||
| break |
There was a problem hiding this comment.
Same focus-management issue as above: this branch attempts to move focus from a substep back to the parent step via ref.current?.focus(), but ref is a function ref in current usage so this never runs. Use a local ref for the parent step element (merged with the forwarded ref) and focus that local ref on ArrowLeft/ArrowRight.
| } | ||
| aria-disabled={step.disabled ? 'true' : undefined} | ||
| aria-label={clickableStepLabel} | ||
| aria-labelledby={stepTitleId} |
There was a problem hiding this comment.
Same issue in the vertical step title control: both aria-label and aria-labelledby are set, so the computed accessible name will come from aria-labelledby and the richer clickableStepLabel (including status) will be ignored. Consider removing one to make the accessible naming intentional and predictable.
| aria-labelledby={stepTitleId} |
| export type StepperV2Step = { | ||
| id: number | ||
| title: string | ||
| status?: StepperV2StepStatus | ||
| disabled?: boolean | ||
| description?: string | ||
| icon?: ReactNode | ||
| substeps?: SubStep[] | ||
| isExpandable?: boolean | ||
| isExpanded?: boolean | ||
| } & Omit<HTMLAttributes<HTMLDivElement>, 'className' | 'style' | 'id'> |
There was a problem hiding this comment.
StepperV2Step allows arbitrary HTMLAttributes<HTMLDivElement> via the type intersection, but those attributes are never spread onto any rendered element in either HorizontalStepComponent or VerticalStepComponent. This is misleading for consumers (e.g., data-testid, onMouseEnter, etc. will be accepted by TS but ignored at runtime). Either remove the HTMLAttributes from the step type, or deliberately spread the remaining step props onto the appropriate step container/interactive element (making sure to avoid clobbering the generated DOM id).
| import { FoundationTokenType } from '../../tokens/theme.token' | ||
| import { ResponsiveStepperV2Tokens } from './stepperV2.tokens' | ||
|
|
||
| export const getStepperV2DarkTokens = ( | ||
| foundationToken: FoundationTokenType | ||
| ): ResponsiveStepperV2Tokens => { | ||
| return { | ||
| sm: { | ||
| container: { | ||
| gap: 6, |
There was a problem hiding this comment.
getStepperV2DarkTokens currently appears to be identical to getStepperV2LightTokens (same token values and structure). If this is intentional, consider deduplicating into a shared base token map to avoid maintaining two ~900-line files; if it’s not intentional, the dark tokens likely need theme-appropriate color/contrast adjustments.
| const handleHorizontalStepClick = (index: number) => { | ||
| console.log('horizontal step clicked', index) | ||
| setHorizontalSteps((prev) => | ||
| prev.map((step, i) => { | ||
| if (i < index) { | ||
| return { | ||
| ...step, | ||
| status: | ||
| step.status === StepperV2StepStatus.COMPLETED | ||
| ? StepperV2StepStatus.COMPLETED | ||
| : StepperV2StepStatus.SKIPPED, | ||
| } | ||
| } | ||
| if (i > index) { | ||
| return { | ||
| ...step, | ||
| status: | ||
| step.status === StepperV2StepStatus.COMPLETED | ||
| ? StepperV2StepStatus.COMPLETED | ||
| : StepperV2StepStatus.PENDING, | ||
| } | ||
| } | ||
| if (i === index) { | ||
| return { | ||
| ...step, | ||
| status: StepperV2StepStatus.CURRENT, | ||
| } | ||
| } | ||
|
|
||
| return step | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
| console.log({ horizontalSteps }) | ||
|
|
||
| const substepsAllPending = (step: StepperV2Step) => |
There was a problem hiding this comment.
This demo file includes console.log statements (e.g., in handleHorizontalStepClick and logging horizontalSteps). If this demo ships to production builds of the site, these logs will add noise and can leak interaction details; consider removing them or gating behind a dev-only flag.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flexDirection="column" | ||
| gap={stepperTokens.container.gap} | ||
| role={isClickable ? 'button' : 'group'} | ||
| tabIndex={isClickable ? 0 : -1} | ||
| aria-current={isCurrent ? 'step' : undefined} |
There was a problem hiding this comment.
Focus styles are applied via _focus on the inner circle Block, but that element isn’t focusable—focus lands on the outer Block (role="button", tabIndex={0}). This means keyboard users likely won’t get a visible focus indicator. Consider moving outline styles to the focusable outer Block (prefer _focusVisible) or styling the circle via :focus-visible/:focus-within.
| _hover={ | ||
| isClickable | ||
| ? { | ||
| backgroundColor: | ||
| stepperTokens.container.step |
There was a problem hiding this comment.
Similar to the horizontal implementation, _focus outline styles are applied to the circle Block, but that element isn’t focusable. Keyboard focus lands on the step content Block with tabIndex={0} (when clickable), so the intended focus ring likely never appears. Consider moving these focus styles to the focusable step content wrapper (prefer _focusVisible) or using :focus-within to style the circle when the step content is focused.
There was a problem hiding this comment.
here make a clean storybook also always add visual, accessibility and interaction story
There was a problem hiding this comment.
remove this file change
| active: { | ||
| default: { | ||
| color: CSSObject['color'] | ||
| height: CSSObject['height'] | ||
| } | ||
| } | ||
| inactive: { |
There was a problem hiding this comment.
this active and inactive is state then it should not be define directly
vinitkhandal717
left a comment
There was a problem hiding this comment.
don't use hardcode value also i see vertical and horizontal stepper may code is repeated can we extract the common code as in horizontal and vertical stepper only difference is just the layout
| color?: string | ||
| height?: string | ||
| } | ||
| >(({ color = FOUNDATION_THEME.colors.gray[300], height = '100%' }, ref) => { |
There was a problem hiding this comment.
why we use the hardcode value here
| return ( | ||
| <Block | ||
| ref={ref} | ||
| width="1px" |
| width="1px" | ||
| height={height} | ||
| backgroundColor={color} | ||
| minHeight={32} |
There was a problem hiding this comment.
don't use hardcode height
0674ead to
74caa5e
Compare
Summary
Screen.Recording.2026-03-31.at.7.26.29.PM.mov
Issue Ticket
Closes #1305