Add new stepper component and polish the design on the use in your ap…#2480
Add new stepper component and polish the design on the use in your ap…#2480sarah-inkeep merged 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 25adbdf The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Introduces a reusable
|
There was a problem hiding this comment.
Clean refactor. The new stepper component has a clear composable API, the docsPath/docsLabel prop lift to the modal footer is correct, and the Streamdown usage for the code snippet is consistent with existing patterns in the ship guides. No issues found.
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
stepper.tsx:4-6Missingdata-slotattributes — breaks established UI component convention
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
use-in-your-app-section.tsx:97-103Step 2 code block lacks copy button (inconsistent with Step 1)
💭 Consider (2) 💭
💭 1) stepper.tsx Use semantic HTML for accessibility
Issue: The Stepper component uses non-semantic <div> elements for what is semantically an ordered list of steps.
Why: Screen readers won't announce this as a list or convey the step count. Users navigating by list landmarks won't find this content. The web-design-guidelines skill recommends semantic HTML before ARIA.
Fix: Refactor Stepper to use <ol> and StepperItem to use <li> with list-none class to preserve visual styling while improving accessibility.
💭 2) component-name-utils.ts Pre-existing: Utility functions lack test coverage
Issue: toPascalCase and extractExportedComponentName utilities (used by the refactored section component) lack unit tests despite containing regex-based parsing logic.
Why: These functions generate CLI commands and import snippets that users copy into their codebases. If the regex fails to extract names correctly, users get broken code snippets.
Fix: Consider adding tests in agents-manage-ui/src/lib/__tests__/component-name-utils.test.ts for edge cases (empty strings, special characters, various export patterns).
Refs: component-name-utils.ts
🚫 REQUEST CHANGES
Summary: The new Stepper component is well-designed and follows the composition pattern used elsewhere, but it's missing the data-slot attributes that are consistently used across all other compound UI components in this codebase. Adding these attributes ensures the component integrates properly with existing CSS targeting patterns. The copy button regression in Step 2 is a minor UX issue worth addressing.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
stepper.tsx:1 |
React import style diverges from majority pattern (import type { ComponentProps } vs import type * as React from 'react') |
Not significant — there's existing split in the codebase (tabs.tsx uses same pattern) |
use-in-your-app-modal.tsx:44 |
Removal of max-height constraint could cause overflow | Low confidence — Dialog component handles scrolling via its own constraints |
use-in-your-app-section.tsx:32 |
buildImportAndRegistrationSnippet could benefit from tests |
Low confidence (INFO severity) — simple string concatenation with visible output |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-consistency |
2 | 0 | 0 | 0 | 1 | 0 | 1 |
pr-review-product |
2 | 0 | 0 | 0 | 1 | 0 | 1 |
pr-review-frontend |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
pr-review-tests |
2 | 0 | 1 | 0 | 0 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 7 | 0 | 2 | 0 | 2 | 0 | 3 |
| function Stepper({ className, ...props }: ComponentProps<'div'>) { | ||
| return <div className={cn('relative', className)} {...props} />; | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Missing data-slot attributes — breaks established UI component convention
Issue: The Stepper component does not include data-slot attributes, which are used consistently across all compound UI components in this codebase for component identification and CSS targeting.
Why: All other compound UI components (Card, Dialog, Sheet, Alert, Table, Popover, AlertDialog, etc.) include data-slot attributes on every sub-component. This is a first-of-its-kind pattern for a new compound UI component, so it sets precedent. Without these attributes, CSS selectors like [data-slot="stepper-title"] won't work, breaking the established component API pattern.
Fix: Add data-slot attributes to all Stepper sub-components:
| function Stepper({ className, ...props }: ComponentProps<'div'>) { | |
| return <div className={cn('relative', className)} {...props} />; | |
| } | |
| function Stepper({ className, ...props }: ComponentProps<'div'>) { | |
| return <div data-slot="stepper" className={cn('relative', className)} {...props} />; | |
| } |
Similarly for other components:
StepperItem→data-slot="stepper-item"StepperIndicator→data-slot="stepper-indicator"StepperContent→data-slot="stepper-content"StepperTitle→data-slot="stepper-title"StepperDescription→data-slot="stepper-description"
Refs:
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Prior Feedback Addressed
The issues raised in the previous review have been resolved:
| Prior Issue | Resolution |
|---|---|
🟠 Major: Missing data-slot attributes |
✅ Fixed in commit a870833e0 — all stepper sub-components now include data-slot attributes following the kebab-case naming convention (stepper, stepper-item, stepper-indicator, stepper-content, stepper-title, stepper-description) |
| 🟡 Minor: Step 2 code block lacks copy button | ✅ Author clarified: Streamdown has built-in copy functionality for code blocks |
💭 Consider (carried forward)
These remain valid suggestions from the prior review but are not blocking:
💭 1) stepper.tsx Use semantic HTML for accessibility
Issue: The Stepper component uses <div> elements where semantic <ol>/<li> would improve screen reader support.
Why: Screen readers won't announce this as a list or convey the step count without semantic HTML.
Fix: Consider refactoring to use <ol> and <li> with list-none class in a future iteration.
💭 2) component-name-utils.ts Utility functions lack test coverage (pre-existing)
Issue: toPascalCase and extractExportedComponentName utilities lack unit tests despite containing regex-based parsing logic.
Why: These functions generate CLI commands and import snippets that users copy — edge case failures could produce broken snippets.
Fix: Consider adding tests for edge cases in a follow-up.
✅ APPROVE
Summary: The feedback from the prior review has been addressed. The data-slot attributes now follow the established convention used across other compound UI components (Card, Sheet, Dialog, etc.). Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta re-review — prior issues addressed, no new reviewers dispatched.
…p modal