Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Match lightning-progress-indicator:
- `type="path"` renders the steps as SLDS Path stages: a listbox of
`a[role=option]` stages with complete/current/incomplete status,
"Stage Complete" / "Current Stage:" assistive text, and a visible
"Stage: {name}" label. The stages are a single tab stop; the arrow
keys move between them, and the tab stop returns to the selected
step when `selectedStep` changes. New `assistiveText.completedStage`,
`assistiveText.currentStage`, and `labels.stageNameBefore` keys.
- `variant="shade"` adds `slds-progress_shade` to the base type.
- Dev-only warnings when `type="path"` is combined with base-only props
(`errorSteps`, `disabledSteps`, `orientation="vertical"`, tooltip
props, non-base `variant`).
- New `useRovingTabIndex` hook in `utilities/hooks`.
- Fix `incompatibleProps` only matching the last value in a values list.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
showerbee
left a comment
showerbee
left a comment
There was a problem hiding this comment.
Thanks for this. It's well scoped, the description is thorough, and keeping type separate from variant matches LWC. The non-breaking reasoning holds up too. I checked out the branch, ran the tests (all pass, CI green), and probed some edge cases. Details are in the inline comments. Here's the summary:
Should fix before merge
- The regression test for the
incompatiblePropsfix doesn't exercise the fix. I restored master'sincompatible-props.jsand the progress-indicator and popover tests still pass (44/44). See the inline comment. - The stage-name wrapper is missing
slds-path__action, which the SLDS Path blueprint uses and which carries the responsive layout in the SLDS 2 CSS. aria-selectedfollows focus, not the selected step. After the arrow keys and a Tab out, assistive tech hears a different "selected" stage than the one shown as current, and the listbox label disagrees with both.useRovingTabIndexbecomes public API as soon as this ships, becausepackage.jsonexports./utilities/*. I'd like to settle its API (or keep it private) before release.
Should consider: false-positive warnings when disabledSteps or errorSteps is [], warnings that only run on mount, the listbox name when selectedStep isn't in steps, and Enter/Space behavior on role="option".
Not in the diff, so noting it here:
- Docs site:
component.jsonsite-storiesand__examples__/weren't updated, so the path type only appears in Storybook. Consider adding__examples__/path.jsxand asite-storiesentry. - Accessibility review:
component.jsonshows the last accessibility review was in 2018. This adds a whole new interaction model (a listbox with roving focus), so I'd like an accessibility reviewer to approve before merge. - Commits: consider moving the
incompatiblePropsfix into its ownfix(warning):commit so it shows up as a fix in the changelog, not only inside thefeatcommit. - Missing tests: a stage that's both completed and selected (the description says it renders as current, which is worth pinning), and the narrow-width stage-name layout.
I'd treat items 1–3 as blocking. Item 4 is for maintainers to discuss.
| renderProgressIndicator({ | ||
| ...pathProps, | ||
| errorSteps: [steps[2]], | ||
| variant: 'shade', |
There was a problem hiding this comment.
This test doesn't guard the incompatibleProps fix. 'shade' is the last value in ['modal', 'setup-assistant', 'shade'], and the old forEach bug only got the last value right, so the old code passes too. I checked by restoring master's utilities/warning/incompatible-props.js: every progress-indicator and popover test still passes.
Suggestions:
- Use
variant: 'modal'or'setup-assistant'here. - Better: add a direct unit test for
incompatiblePropswith a list of several values, covering both the first and the middle item. The fix is in a shared utility, so its test shouldn't depend on one component's prop list.
| let propOneConditionMet = false; | ||
| let propTwoConditionMet = false; | ||
|
|
||
| if (props[propOneName] && props[propTwoName]) { |
There was a problem hiding this comment.
This is a truthiness check, so [] counts as "passed." type="path" with disabledSteps={[]} or errorSteps={[]} logs both warnings. I verified this. Passing empty arrays is common when a consumer shares one set of props between both types.
I wouldn't change this shared check. Instead, check-props.js could pass undefined for empty arrays, or check .length for the step-array props.
| checkProps = function checkPropsFunction(COMPONENT, props) { | ||
| const comment = 'Only `type="base"` supports this prop.'; | ||
| const baseOnlyProps = [ | ||
| ['disabledSteps', null], |
There was a problem hiding this comment.
Related to the comment on incompatible-props.js: disabledSteps, errorSteps, and tooltipIsOpenSteps with null values warn on any truthy value, including []. Consider normalizing empty arrays to undefined before calling incompatibleProps.
| const allSteps = checkSteps(propSteps) ? propSteps : defaultSteps; | ||
|
|
||
| // Check props in development | ||
| useEffect(() => { |
There was a problem hiding this comment.
Two small points:
- This only runs on mount, so rendering
type="base" variant="modal"and then re-rendering astype="path"logs nothing (verified). That matches the repo's existing check-props pattern, so it's a nit, but listing the checked props as dependencies would be cheap. selectedStepandsteps(lines 227–228) are passed in, butcheck-props.jsnever reads them.
| {/* eslint-disable-next-line jsx-a11y/anchor-is-valid */} | ||
| <a | ||
| {...itemProps} | ||
| aria-selected={index === activeIndex} |
There was a problem hiding this comment.
aria-selected follows the roving activeIndex (focus), but slds-is-current, the "Current Stage:" assistive text, and the listbox label (Stage: {currentStep}) all follow isSelected.
Repro (verified): selected stage is 2, focus it, press ArrowRight twice, then Tab out. The result:
aria-selected="true"on stage 4slds-is-currentand "Current Stage:" on stage 2- the listbox's name is "Stage: <stage 2>"
- no
onStepClickfires, so app state doesn't change either
A screen reader user hears stage 4 as selected, which contradicts everything else. If this is a deliberate match with LWC's progressStep, please cite that code in the PR description and add a test that pins this behavior. If it isn't, aria-selected should probably follow isSelected, with slds-is-active and tabIndex following focus.
| </div> | ||
| <div className="slds-grid"> | ||
| <span id={stageNameId} className="slds-path__stage-name"> | ||
| {labels.stageNameBefore} {currentStep?.label} |
There was a problem hiding this comment.
Two points:
- If
selectedStepisn't insteps, this renders"Stage: ", so the listbox's accessible name is just "Stage:". In that case the tab stop also falls back to index 0 (verified). It's an edge case, but worth a fallback or at least a test. - The hard-coded space between
stageNameBeforeand the label doesn't suit every locale. A format string, or lettingstageNameBeforeinclude its own spacing, would be easier to translate.
| /** Number of items in the group */ | ||
| count: number; | ||
| /** Index of the item that holds the tab stop at first, and again whenever this value changes */ | ||
| defaultIndex?: number; |
There was a problem hiding this comment.
package.json exports ./utilities/*, so this hook becomes public API (@salesforce/design-system-react/utilities/hooks) as soon as this ships. Under the breaking-change rules, renaming it later is breaking. So I'd push on the API now:
- Naming: in React,
default*means "initial value, then uncontrolled." This value resets the active index whenever it changes, which is neither uncontrolled nor fully controlled. ConsiderselectedIndexorsyncIndex, or a fully controlledactiveIndexwithonActiveIndexChange. - Scope: with one consumer, there's no disabled-item skipping, no Home/End, no wrap option, no RTL, and both arrow axes are handled regardless of orientation. Tabs, tree, carousel indicators, and pill-container already have their own keyboard handling. I'd rather keep this in
progress-indicator/private/until a second consumer shows what the shared API should be. If it stays shared, it needs its own unit tests; today it's only tested through Path.
| export interface RovingTabIndexItemProps<T extends HTMLElement> { | ||
| ref: (node: T | null) => void; | ||
| tabIndex: 0 | -1; | ||
| onFocus: () => void; |
There was a problem hiding this comment.
The prop getter doesn't compose handlers. onFocus takes no event, and callers have to override it and remember to call itemProps.onFocus(), as path.tsx does. If a caller spreads getItemProps(index) after its own onKeyDown or onFocus, the caller's handler is silently replaced.
The usual pattern (as in downshift) is getItemProps(index, { onFocus, onKeyDown, ref }), which calls the caller's handler and then the hook's, with the event passed through.
| }; | ||
|
|
||
| const getItemProps = (index: number): RovingTabIndexItemProps<T> => ({ | ||
| ref: (node) => { |
There was a problem hiding this comment.
Nit: this creates a new ref callback for every item on every render, so React detaches and re-attaches every item ref (null, then the node) on each render. It's harmless, but a ref callback cached per index (or a stable callback that reads index from a data attribute) avoids the churn.
| describe('SLDSProgressIndicator path (browser)', () => { | ||
| const renderPath = () => | ||
| render( | ||
| <IconSettings iconPath="/assets/icons"> |
There was a problem hiding this comment.
Two points:
iconPath="/assets/icons"probably causes 404 requests for the sprite in browser mode. It's harmless but noisy.- Worth adding a browser test for the arrow-then-Tab-out case: arrow to a stage that isn't current, Tab out, Shift+Tab back, and assert which stage has focus and
aria-selected. That pins down whichever behavior we settle on in thearia-selectedthread onpath.tsx.
The regression test for the incompatibleProps fix only checked the last value in the list, which the old code also got right. Test the first, middle, and last items for both props directly, and run utilities/**/__tests__ in the unit project. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Wrap the stage name in `slds-path__action`, as in the SLDS blueprint, so it lays out correctly at narrow and medium widths - Move useRovingTabIndex to progress-indicator/private so it doesn't become public API through `./utilities/*`; rename `defaultIndex` to `selectedIndex`, compose caller handlers, and keep item ref callbacks stable - Focus a stage on click so it takes the tab stop in Safari - Let `labels.stageNameBefore` carry its own spacing and add `labels.stageNameAfter`, instead of a hard-coded space - Don't warn for empty step arrays, and re-run the dev checks when the checked props change - Add tests for the variant list's first and middle values, a stage that is both completed and selected, a selectedStep missing from steps, and aria-selected after arrowing and tabbing away (which follows the SLDS Path spec); load real icon sprites in browser tests - Add a Path example to the docs site Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
showerbee
left a comment
showerbee
left a comment
There was a problem hiding this comment.
All major issues look addressed- looks good!
This branch has not been deployed
2 participants
Additional description
Adds
type="path"andvariant="shade"toProgressIndicator, matchingtypeandvariantonlightning-progress-indicator. I checked the behavior against theprogressIndicatorandprogressStepsource inlightning-base-components, and the markup against the SLDS Path blueprint.variant="shade"addsslds-progress_shadeto the progress bar, the same as LWC.modalstill adds the shade class and keeps its absolute tooltip positioning, so modal markup doesn't change.type="path"renders the steps as SLDS Path stages:listboxofa[role=option]stages, withslds-is-complete,slds-is-current,slds-is-incomplete, andslds-is-activeclasses and the check icon.assistiveText.completedStageandassistiveText.currentStagekeys and newlabels.stageNameBeforeandlabels.stageNameAfterkeys;step.assistiveTextstill overrides per stage. The labels carry their own spacing (the default is'Stage: '), so translations can put the name anywhere.div.slds-grid.slds-path__action, as in the SLDS blueprint (LWC uses a bareslds-grid). Measured in Chromium, the class centers the name in a full-width row at narrow widths and adds the blueprint's 16px gap at medium widths, where it otherwise touches the track. The cost is a 24px margin at the end of the track at ≥64em, where the name is hidden; the blueprint fills that space with its Mark Complete button.selectedStepchanges, the tab stop moves back to it. Clicking a stage focuses it, so it takes the tab stop in Safari too, where clicks don't focus links.aria-selectedfollows the active stage (the one the user has moved to), not the current stage. The SLDS Path accessibility docs (ui/components/path/docs.mdxin the SLDS repo) specify this: "aria-selected="true"is used to describe the currently activeslds-path__linkTab, not the Stage the Path is currently set to", and the arrow keys "move focus and selection". LWC does the same:lightning-progress-stepsets botharia-selectedandtabindexfromstate.active.slds-is-current, the "Current Stage:" text, and the listbox name stay on the step the app selected.onStepClickandonStepFocusreceive the samedatashape as the base type. Clicks callpreventDefault(), because the link ishref="#". Enter activates a stage through the link's native click; Space does nothing. That matches LWC, which has no activation handling on path stages, and SLDS's keyboard spec, which only covers the arrow keys.selectedStepisn't insteps, no stage is current, the first stage holds the tab stop, and the listbox name is just "Stage:". The base type also falls back to the first step.Differences from LWC
completedSteps, as they do for the base type, not from each stage's position before the current step. A stage that is both completed and selected renders as current.slds-is-rtl), and stage coaching. LWC has no coaching or Home/End either.Base-only props.
errorSteps,disabledSteps,orientation,tooltipIsOpenSteps,tooltipPosition, and non-basevariantvalues don't apply to the path. Combining them withtype="path"logs a dev-only warning via a newcheck-props.js. Empty step arrays don't count as passed, so props shared between both types don't warn. The check runs again when one of those props changes, but not when a step array is only a new array with the same contents. Their JSDoc now says "type="base"only". I didn't narrow the props into a discriminated union, because the breaking-change taxonomy counts type narrowing as breaking.Other changes
components/progress-indicator/private/use-roving-tab-index.ts).package.jsonexports./utilities/*, so putting it inutilities/hooks/would make it public API with one consumer. It can move there once a second component shows what the shared API needs.getItemProps(index, { onFocus, onKeyDown })calls the caller's handlers before its own, and the item ref callbacks are stable across renders.utilities/warning/incompatible-props.js: when given a list of incompatible values, it only checked the last one, because aforEachoverwrote the match flag on each iteration. Popover, the only other caller, passes single values, so its warnings don't change. The new path warnings depend on the fix (variantis checked againstmodal,setup-assistant, andshade). This repo squash-merges, so the changelog entry for the fix belongs under "Fixed" in the release notes.vitest.config.ts: theunitproject now also runsutilities/**/__tests__/*.test.*, for the newincompatiblePropstest. No existing file matched that pattern.Not a breaking change:
typeand the newassistiveTextandlabelskeys are optional, and the defaults render the same markup as before. Existing story snapshots are unchanged; the snapshot diff only adds the three new stories. Nothing is added toutilities/hooks.Commits:
style(progress-indicator)is formatting only. The pre-commit hook formats whole files, and three of them weren't formatted on master. Thefeatcommit has the functional diff, and the later commits address review feedback.Tests and stories
progress-indicator.test.jsx): the shade class, plus for the path:selectedStepthat isn't instepsaria-selected, arrow-key navigation and the end stops, the tab stop resetting whenselectedStepchanges, and click moving the tab stopvariantvalue, including the first and middle items of the list; no warning for empty arrays or compatible values; warning after a rerender into an incompatible combination; no repeat warning when only an array's identity changesutilities/warning/__tests__/incompatible-props.test.js): the first, middle, and last items of a value list, for both props. With master'sincompatible-props.js, the first and middle cases fail; with the fix, all pass.progress-indicator.browser.test.jsx), with the SLDS 2 Lightning Blue stylesheet and real icon sprites loaded:aria-selected, while the current stage and listbox name stay putShadeVariant,Path, andInteractivePathstories, plus atypecontrol andshadein thevariantcontrol.__examples__/path.jsxand a "Path"site-storiesentry incomponent.json.CONTRIBUTOR checklist (do not remove)
Please complete for every pull request.
Green gate — all of these must pass locally (CI runs the same):
npm run lintpasses with 0 errors (npm run lint:fixto auto-fix). Warnings are the tracked gradual-migration backlog; do not add new errors.npm run typecheckis clean (tsc --noEmit).npm test -- --runpasses — this runs both Vitest projects (jsdomunit+ real-browserbrowser) and the story HTML snapshots. If you changed rendered markup on purpose, update snapshots withnpm test -- --run -uand review the diff.npm run buildsucceeds (type-check + Vite library build +.d.ts+ entry shims).Behavior & docs:
components/button/__tests__). New props and bug fixes should get a regression test. Tests that genuinely need a real browser (focus traversal, layout/measurement, canvas) go in a*.browser.test.jsxfile (thebrowserproject); everything else is jsdom.npm run storybook→ http://localhost:6007/. Confirm the story actually renders and behaves (deprecated Storyshots/Karma are gone; there is no separate browser-tests server).onRenderContent/onRenderVisualPicker,iconprops,events), not assumptions — a common source of "component looks broken" reports has been stories authored against the wrong API. Cross-check againstcomponents/<name>/__examples__..d.ts(they're generated by the build); JS consumers rely on them via editor type-acquisition.REVIEWER checklist (do not remove)
CIworkflow: lint, typecheck, test, build). Chromatic visual review, if configured, has been checked.components/__tests__/__snapshots__/story-snapshots.test.jsx.snap).@salesforce/design-system-react/components/<name>and nested paths resolve for ESM, CJS, and types). Removing/renaming a public component or export is a breaking change →feat!:/BREAKING CHANGE:and a major version bump.