refactor button and visually hidden to forward refs #1414
Conversation
|
Warning Rate limit exceeded@kotAPI has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughThis PR removes the VisuallyHidden component and all related docs, stories, tests, and navigation/SEO entries, updates the released components list, and refactors the Button to forward refs to ButtonPrimitive with refined typings. It also strengthens ButtonPrimitive’s TypeScript types and adds a ref-forwarding test for Button. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Consumer Component
participant Btn as Button (UI)
participant Prim as ButtonPrimitive
participant DOM as <button>
User->>Btn: render(<Button ref={r} .../>)
Note over Btn: compute data/accent attrs
Btn->>Prim: render props + ref
Prim->>DOM: render <button type, aria-*>
Btn-->>User: ref forwarded to DOM
sequenceDiagram
autonumber
participant Prim as ButtonPrimitive
participant DOM as <button>
Prim->>DOM: apply type="button" (default)
alt has label
Prim->>DOM: set aria-label
end
alt disabled && no description
Prim->>DOM: set aria-description="Disabled Button"
else disabled && description
Prim->>DOM: set aria-description=description
end
alt disabled
Prim->>DOM: set aria-disabled=true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| type VisuallyHiddenElement = React.ElementRef<typeof Primitive.div>; | ||
| type VisuallyHiddenProps = React.ComponentPropsWithoutRef<typeof Primitive.div> & { | ||
| customRootClass?: string; | ||
| className?: string; | ||
| asChild?: boolean; | ||
| style?: CSSProperties; |
There was a problem hiding this comment.
[P1] Restore VisuallyHiddenProps export
The refactor changes VisuallyHiddenProps to a local type and drops the export keyword. VisuallyHiddenProps is still imported from this module in the Storybook file (VisuallyHidden/stories/VisuallyHidden.stories.tsx), so the module will no longer compile because the named export is missing. Please keep the type exported so downstream code and the stories can continue to import it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/components/ui/Button/tests/Button.test.tsx (1)
46-51: Good ref-forwarding assertion; consider adding a callback-ref case and stronger identity check.
- Add a callback-ref test to cover both ref patterns.
- Optionally assert identity against the rendered node via getByRole for extra confidence.
+ test('supports callback ref', () => { + let node: HTMLButtonElement | null = null; + render(<Button ref={(el) => { node = el; }}>cb ref</Button>); + expect(node).toBeInstanceOf(HTMLButtonElement); + expect(node?.tagName).toBe('BUTTON'); + }); + + test('ref points to the rendered element', () => { + const ref = React.createRef<HTMLButtonElement>(); + render(<Button ref={ref}>ref identity</Button>); + const btn = screen.getByRole('button', { name: 'ref identity' }); + expect(ref.current).toBe(btn); + });src/core/primitives/Button/index.tsx (3)
29-30: Avoid defaulting role='button' on a native button.Role is already implicit; defaulting it adds noise and can mask misuse. Let consumers opt-in if they truly need a non-default role.
-const ButtonPrimitive = React.forwardRef<ButtonPrimitiveElement, ButtonPrimitiveProps>(({ role = 'button', type = 'button', label = '', description = '', disabled = false, children, ...props }, ref) => { +const ButtonPrimitive = React.forwardRef<ButtonPrimitiveElement, ButtonPrimitiveProps>(({ role, type = 'button', label = '', description = '', disabled = false, children, ...props }, ref) => {
43-49: Skip redundant aria-disabled on native buttons (or at least use boolean).For a native , the disabled attribute suffices; aria-disabled is redundant and may trigger a11y linters. If you keep it for polymorphic cases, set it as a boolean.
Option A (recommended): remove the block
- if (disabled) { - // If the button is disabled, we should set the aria-disabled attribute - props['aria-disabled'] = 'true'; - if (!description) { // If description isn't set, we set a default description - props['aria-description'] = 'Disabled Button'; - } - }Option B (minimal): boolean value and no auto-description
- if (disabled) { - props['aria-disabled'] = 'true'; - if (!description) { - props['aria-description'] = 'Disabled Button'; - } - } + if (disabled && props['aria-disabled'] === undefined) { + props['aria-disabled'] = true; + }
37-42: Optional: Prefer aria-describedby over aria-description for broader AT support.aria-description has varying support; aria-describedby pointing to hidden helper text is still the most interoperable path.
src/components/ui/Button/Button.tsx (2)
19-19: Nit: avoid duplicating default type at both layers.ButtonPrimitive already defaults type='button'. You can drop the wrapper default to reduce duplication.
-const Button = React.forwardRef<ButtonElement, ButtonProps>(({ children, type = 'button', customRootClass = '', className = '', variant = '', size = '', color = '', ...props }, ref) => { +const Button = React.forwardRef<ButtonElement, ButtonProps>(({ children, type, customRootClass = '', className = '', variant = '', size = '', color = '', ...props }, ref) => {
11-17: Consider exporting ButtonProps and narrowing variant/size/color to unions.If consumers type against Button, exporting ButtonProps helps. Constraining variant/size/color to known unions improves DX and prevents invalid attributes.
Would any external code import ButtonProps today? If yes, I can generate a follow-up PR to export it and introduce unions like 'variant: "solid" | "outline" | "ghost"' and 'size: "small" | "medium" | "large"'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
docs/app/docs/components/visually-hidden/docs/codeUsage.js(0 hunks)docs/app/docs/components/visually-hidden/page.mdx(0 hunks)docs/app/docs/components/visually-hidden/seo.ts(0 hunks)docs/app/docs/docsNavigationSections.tsx(0 hunks)scripts/RELEASED_COMPONENTS.cjs(0 hunks)src/components/ui/Button/Button.tsx(4 hunks)src/components/ui/Button/tests/Button.test.tsx(1 hunks)src/components/ui/VisuallyHidden/VisuallyHidden.tsx(0 hunks)src/components/ui/VisuallyHidden/stories/VisuallyHidden.stories.tsx(0 hunks)src/components/ui/VisuallyHidden/tests/VisuallyHidden.test.js(0 hunks)src/core/primitives/Button/index.tsx(2 hunks)
💤 Files with no reviewable changes (8)
- src/components/ui/VisuallyHidden/tests/VisuallyHidden.test.js
- docs/app/docs/components/visually-hidden/page.mdx
- scripts/RELEASED_COMPONENTS.cjs
- docs/app/docs/components/visually-hidden/docs/codeUsage.js
- docs/app/docs/components/visually-hidden/seo.ts
- src/components/ui/VisuallyHidden/VisuallyHidden.tsx
- src/components/ui/VisuallyHidden/stories/VisuallyHidden.stories.tsx
- docs/app/docs/docsNavigationSections.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/Button/Button.tsx (1)
src/core/primitives/Primitive/Primitive.stories.tsx (1)
Button(20-25)
🔇 Additional comments (2)
src/core/primitives/Button/index.tsx (1)
20-27: Type aliases and prop surface look solid.Deriving the element and props from Primitive.button and reintroducing a narrowed type prop is correct and future-proof.
src/components/ui/Button/Button.tsx (1)
19-37: Ref forwarding and composed data-attributes look good.Ref is correctly forwarded to ButtonPrimitive; attribute composition is clean and isolated.
Summary
Testing
npm testSummary by CodeRabbit