Add DropdownButton and rename ButtonWithIcon into ButtonIcon#105
Open
Add DropdownButton and rename ButtonWithIcon into ButtonIcon#105
Conversation
Contributor
WalkthroughA new ChangesLanguage Selection Dropdown
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/button-icon.tsx (1)
4-22: ⚡ Quick winNo HTML button props are forwarded — the component cannot respond to interactions.
The current signature only accepts
variant,icon,text, andiconPosition. There is no way for callers to passonClick,disabled,type,className, or anyaria-*/data-*attribute. In practice this makesButtonIconnon-interactive and inaccessible as a standalone component.♻️ Proposed fix: forward rest props to
Button-export function ButtonIcon({ +export function ButtonIcon({ variant = "primary", icon: Icon, text, iconPosition = "left", + ...props }: { variant?: "primary" | "tertiary" | "tertiaryBlur" icon: IconType text: string iconPosition?: "left" | "right" -}) { +} & React.ComponentProps<"button">) { return ( - <Button variant={variant} size="lg"> + <Button variant={variant} size="lg" {...props}> {iconPosition === "left" && <Icon />} {text} {iconPosition === "right" && <Icon />} </Button> ) }Also add
import type * as React from "react"(orimport React from "react") at the top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/button-icon.tsx` around lines 4 - 22, ButtonIcon currently only accepts variant, icon, text and iconPosition so callers cannot pass interactions or attributes; update ButtonIcon to accept and forward arbitrary button props by extending the appropriate button props type (e.g., React.ButtonHTMLAttributes<HTMLButtonElement> or the Button component's prop type) and collect the rest with a rest pattern (e.g., ...rest) and pass them into the rendered <Button> (the Button component) so onClick, disabled, type, className, aria-* and data-* work; also add the React type import (import type * as React from "react" or equivalent) so the prop type references compile.src/components/button-dropdown.tsx (1)
14-14: 💤 Low valueConsider deriving the
sizetype fromSelectTriggerto stay DRY.The
"default" | "sm"union is already defined as a custom prop onSelectTrigger. If that definition ever changes,ButtonDropdownPropssilently diverges.♻️ Proposed refactor
+import type React from "react" import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@/components/ui/select"- size?: "default" | "sm" + size?: React.ComponentProps<typeof SelectTrigger>["size"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/button-dropdown.tsx` at line 14, The ButtonDropdownProps currently repeats the size union ("default" | "sm") and should derive it from SelectTrigger to avoid divergence; update the size definition in ButtonDropdownProps to reference the existing prop type on SelectTrigger (e.g., use React.ComponentProps<typeof SelectTrigger>['size'] or the appropriate exported prop type) so ButtonDropdownProps.size is always driven by SelectTrigger's type; adjust imports if necessary to access SelectTrigger in src/components/button-dropdown.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/page.tsx`:
- Around line 18-24: The DropdownButton in page.tsx is missing value and
onChange wiring so selections never update app state; add a controlled state or
hook to handle language changes: inside the page component (or a client wrapper)
create state (e.g., useState or an i18n/context hook) to hold the selected
language, pass that state as value and an onChange handler to DropdownButton,
and invoke your language-switch logic (or routing/i18n API) from the handler; if
you intentionally defer implementing the handler, add a clear TODO noting that
DropdownButton needs a value/onChange wired to the app's language switcher.
---
Nitpick comments:
In `@src/components/button-dropdown.tsx`:
- Line 14: The ButtonDropdownProps currently repeats the size union ("default" |
"sm") and should derive it from SelectTrigger to avoid divergence; update the
size definition in ButtonDropdownProps to reference the existing prop type on
SelectTrigger (e.g., use React.ComponentProps<typeof SelectTrigger>['size'] or
the appropriate exported prop type) so ButtonDropdownProps.size is always driven
by SelectTrigger's type; adjust imports if necessary to access SelectTrigger in
src/components/button-dropdown.tsx.
In `@src/components/button-icon.tsx`:
- Around line 4-22: ButtonIcon currently only accepts variant, icon, text and
iconPosition so callers cannot pass interactions or attributes; update
ButtonIcon to accept and forward arbitrary button props by extending the
appropriate button props type (e.g.,
React.ButtonHTMLAttributes<HTMLButtonElement> or the Button component's prop
type) and collect the rest with a rest pattern (e.g., ...rest) and pass them
into the rendered <Button> (the Button component) so onClick, disabled, type,
className, aria-* and data-* work; also add the React type import (import type *
as React from "react" or equivalent) so the prop type references compile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d49a7bea-e2a9-4eed-aab2-2fbc3ca3aa88
📒 Files selected for processing (3)
src/app/page.tsxsrc/components/button-dropdown.tsxsrc/components/button-icon.tsx
Comment on lines
+18
to
+24
| <DropdownButton | ||
| placeholder="Select language" | ||
| options={[ | ||
| { label: "Italian", value: "it" }, | ||
| { label: "English", value: "en" }, | ||
| ]} | ||
| /> |
Contributor
There was a problem hiding this comment.
DropdownButton has no onChange/value — language selection is a no-op.
Without onChange and managed value, user selections are swallowed by Radix internals and never reach application state. If the language-switching logic is deferred, add a // TODO comment; otherwise wire up useState (or a context/i18n hook) to make the selection functional.
⚡ Minimal wiring example (client component)
+"use client"
+import { useState } from "react"
import { DropdownButton } from "@/components/button-dropdown"
export default function Home() {
+ const [lang, setLang] = useState<string>()
return (
<main className="w-full">
...
<DropdownButton
placeholder="Select language"
options={[
{ label: "Italian", value: "it" },
{ label: "English", value: "en" },
]}
+ value={lang}
+ onChange={setLang}
/>Note: actual i18n integration (e.g.
next-intl,next/navigationlocale routing) would replace theuseStateplaceholder above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/page.tsx` around lines 18 - 24, The DropdownButton in page.tsx is
missing value and onChange wiring so selections never update app state; add a
controlled state or hook to handle language changes: inside the page component
(or a client wrapper) create state (e.g., useState or an i18n/context hook) to
hold the selected language, pass that state as value and an onChange handler to
DropdownButton, and invoke your language-switch logic (or routing/i18n API) from
the handler; if you intentionally defer implementing the handler, add a clear
TODO noting that DropdownButton needs a value/onChange wired to the app's
language switcher.
Draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #12