Button #34
Button #34
Conversation
WalkthroughDependency versions updated to include new Radix UI components and newer React releases. Two new UI components introduced: Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✨ Finishing touches
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. Comment |
|
Devo aggiungere anche |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/components/ui/buttonWithIcon.tsx (3)
16-16: Consider making the button size configurable.The size is hardcoded to
"lg". Consider accepting asizeprop to allow different button sizes when needed.export function ButtonWithIcon({ variant = "primary", icon: Icon, text, iconPosition = "left", + size = "lg", ...props }: { variant?: "primary" | "tertiary" | "tertiaryBlur" icon: LucideIcon text: string iconPosition?: "left" | "right" + size?: "default" | "sm" | "lg" } & Omit<ComponentPropsWithoutRef<"button"> & { asChild?: boolean }, "children">) { return ( - <Button variant={variant} size="lg" {...props}> + <Button variant={variant} size={size} {...props}> {iconPosition === "left" && <Icon />} {text} {iconPosition === "right" && <Icon />} </Button> ) }
10-10: Consider supporting all Button variants.The variant type only includes
"primary" | "tertiary" | "tertiaryBlur", excluding"outline"and"link"variants that are supported by the underlying Button component. Unless there's a specific reason to restrict these, consider allowing all variants.+import type { VariantProps } from "class-variance-authority" +import { buttonVariants } from "./button" export function ButtonWithIcon({ variant = "primary", icon: Icon, text, iconPosition = "left", }: { - variant?: "primary" | "tertiary" | "tertiaryBlur" + variant?: VariantProps<typeof buttonVariants>["variant"] icon: LucideIcon text: string iconPosition?: "left" | "right" }) {
1-1: Add screen reader context for the icon.Consider whether the icon is decorative or conveys meaning. If it's decorative (since the text is already present), add
aria-hidden="true"to the icon. If it conveys additional meaning, ensure proper labeling.return ( <Button variant={variant} size="lg"> - {iconPosition === "left" && <Icon />} + {iconPosition === "left" && <Icon aria-hidden="true" />} {text} - {iconPosition === "right" && <Icon />} + {iconPosition === "right" && <Icon aria-hidden="true" />} </Button> )src/components/ui/button.tsx (1)
9-9: Consider extracting base button styles for readability.The base className string is very long (focus-visible, aria-invalid states, etc.). While functional, extracting these into named constants or separate CVA layers could improve maintainability.
Example:
const baseButtonStyles = "inline-flex items-center justify-center gap-2 whitespace-nowrap text-sm font-medium transition-all disabled:pointer-events-none disabled:opacity-50" const svgStyles = "[&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0" const focusStyles = "outline-none focus-visible:border-slate-950 focus-visible:ring-slate-950/50 focus-visible:ring-[3px] dark:focus-visible:border-slate-300 dark:focus-visible:ring-slate-300/50" const ariaInvalidStyles = "aria-invalid:ring-red-500/20 dark:aria-invalid:ring-red-500/40 aria-invalid:border-red-500 dark:aria-invalid:ring-red-900/20 dark:dark:aria-invalid:ring-red-900/40 dark:aria-invalid:border-red-900" const buttonVariants = cva( [baseButtonStyles, svgStyles, focusStyles, ariaInvalidStyles].join(" "), { variants: { // ... } } )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(2 hunks)src/app/page.tsx(2 hunks)src/components/ui/button.tsx(1 hunks)src/components/ui/buttonWithIcon.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/ui/buttonWithIcon.tsx (1)
src/components/ui/button.tsx (1)
Button(62-62)
src/components/ui/button.tsx (1)
src/lib/utils.ts (1)
cn(12-14)
src/app/page.tsx (2)
src/components/ui/buttonWithIcon.tsx (1)
ButtonWithIcon(4-22)src/components/ui/button.tsx (1)
Button(62-62)
🔇 Additional comments (5)
src/app/page.tsx (1)
8-11: LGTM! Clear welcome content.The heading and introductory paragraph provide good context for the page.
src/components/ui/button.tsx (3)
7-7: Clarify the TODO comment about variants.The TODO comment suggests the variants might need work. Are you planning to add more variants, or does this need design review?
39-60: Excellent Button implementation with asChild pattern.The component correctly uses Radix Slot for composition, applies proper data attributes for testing/styling, and supports comprehensive variants and sizes. The use of
ComponentProps<"button">ensures all native button props are supported.
13-21: All custom Tailwind classes in the button variants are properly defined and integrated into the design system. Thebg-button-primary,text-text-accent-lightbg,rounded-buttonsM, andbg-blue-tertiary-blurclasses are defined insrc/styles/figma.cssvia@themedirectives, mapping to Figma design variables. Thetypo-label-largeutility is defined insrc/styles/typography.css. No action needed.package.json (1)
16-17: Dependency versions are secure and compatible.All specified versions are patched for known vulnerabilities: React 19.2.3 includes fixes for critical React Server Component vulnerabilities, Zod 4.2.1 has no known security issues, and Radix UI packages are current and support React 19. No action required.
| }, | ||
| "dependencies": { | ||
| "@t3-oss/env-nextjs": "^0.13.8", | ||
| "@radix-ui/react-separator": "^1.1.8", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if react-separator is imported or used anywhere in the codebase
rg -n "@radix-ui/react-separator" --type=ts --type=tsx -g '!package.json'Repository: PoliNetworkOrg/web
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Check if react-separator is imported or used anywhere in the codebase
rg -n "@radix-ui/react-separator" -g '!package.json'Repository: PoliNetworkOrg/web
Length of output: 340
🏁 Script executed:
#!/bin/bash
# Search for any import/usage of react-separator in source files only
rg "react-separator|Separator" --type=js --type=jsx -g '!node_modules' -g '!pnpm-lock.yaml' -g '!package-lock.json' -cRepository: PoliNetworkOrg/web
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Comprehensive search for react-separator usage excluding lock files and node_modules
rg "react-separator|from.*Separator|import.*Separator" --glob='!pnpm-lock.yaml' --glob='!package-lock.json' --glob='!node_modules'Repository: PoliNetworkOrg/web
Length of output: 117
Remove unused @radix-ui/react-separator dependency from package.json.
The @radix-ui/react-separator dependency is declared but not imported or used in any source files in the codebase.
🤖 Prompt for AI Agents
In package.json around line 16, the dependency "@radix-ui/react-separator":
"^1.1.8" is declared but unused; remove this entry from package.json and then
run package manager cleanup (npm install / npm prune or yarn install) to update
lockfile and node_modules accordingly; also run the test/build or a quick
repo-wide search to confirm nothing imports it before committing.
| <ButtonWithIcon variant="primary" icon={Users} iconPosition="left" text="Diventa socio" /> | ||
| <ButtonWithIcon variant="tertiary" icon={Users} iconPosition="right" text="Diventa socio" /> | ||
| <ButtonWithIcon variant="tertiaryBlur" icon={Users} iconPosition="left" text="Diventa socio" /> | ||
| <Button variant="link">Link</Button> |
There was a problem hiding this comment.
Buttons lack functionality - add onClick handlers or href links.
All four buttons have no interactive behavior (no onClick, href, or navigation). This directly relates to your question about adding href/action support. For a functional page, these buttons need actions.
Example implementations:
// For action buttons, add onClick handlers:
<ButtonWithIcon
variant="primary"
icon={Users}
iconPosition="left"
text="Diventa socio"
onClick={() => {/* handle membership signup */}}
/>
// For navigation, use asChild with Next.js Link:
<ButtonWithIcon
variant="tertiary"
icon={Users}
iconPosition="right"
text="Diventa socio"
asChild
>
<Link href="/membership" />
</ButtonWithIcon>Note: This requires implementing the prop forwarding suggested in the buttonWithIcon.tsx review first.
🤖 Prompt for AI Agents
In src/app/page.tsx around lines 15 to 18, the four Button/ButtonWithIcon
instances lack any interactive behavior; add either onClick handlers for actions
or enable navigation via href/Next.js Link (using asChild) so the buttons
perform membership signup or navigate to the membership page. First ensure
ButtonWithIcon/buttonWithIcon.tsx forwards native button/link props (onClick,
href, asChild, etc.) and supports rendering a child Link when asChild is true;
then update these usages to pass an onClick for inline actions or wrap/pass a
Link href for navigation so each button is functional.
| <ButtonWithIcon variant="primary" icon={Users} iconPosition="left" text="Diventa socio" /> | ||
| <ButtonWithIcon variant="tertiary" icon={Users} iconPosition="right" text="Diventa socio" /> | ||
| <ButtonWithIcon variant="tertiaryBlur" icon={Users} iconPosition="left" text="Diventa socio" /> | ||
| <Button variant="link">Link</Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use Next.js Link component for the link button.
The button with variant="link" should use the asChild prop with Next.js Link to enable client-side navigation.
+import Link from "next/link"
<Button variant="link">Link</Button>
+<Button variant="link" asChild>
+ <Link href="/about">Link</Link>
+</Button>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/app/page.tsx around line 18, the Button currently rendered as a plain
button with variant="link" should use Next.js client-side navigation; update the
code to import Link from 'next/link' and render the Button with the asChild
prop, wrapping a Next.js Link element that receives the href (e.g. <Link
href="/target">) so the Button delegates to Link for navigation while retaining
variant="link". Ensure the Link is the direct child of Button and remove any
duplicate href on both elements.
| export function ButtonWithIcon({ | ||
| variant = "primary", | ||
| icon: Icon, | ||
| text, | ||
| iconPosition = "left", | ||
| }: { | ||
| variant?: "primary" | "tertiary" | "tertiaryBlur" | ||
| icon: LucideIcon | ||
| text: string | ||
| iconPosition?: "left" | "right" | ||
| }) { | ||
| return ( | ||
| <Button variant={variant} size="lg"> | ||
| {iconPosition === "left" && <Icon />} | ||
| {text} | ||
| {iconPosition === "right" && <Icon />} | ||
| </Button> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add support for interactive props (onClick, href, etc.) to make buttons functional.
Regarding your question "Devo aggiungere anche href / action?" — Yes, the component currently cannot handle any interactions. It needs to forward common button props like onClick, support the asChild pattern for links, and accept other HTML button attributes.
Apply this diff to add essential prop forwarding:
+import type { ComponentPropsWithoutRef } from "react"
import type { LucideIcon } from "lucide-react"
-import { Button } from "./button"
+import { Button, buttonVariants } from "./button"
+import type { VariantProps } from "class-variance-authority"
export function ButtonWithIcon({
variant = "primary",
icon: Icon,
text,
iconPosition = "left",
+ ...props
}: {
variant?: "primary" | "tertiary" | "tertiaryBlur"
icon: LucideIcon
text: string
iconPosition?: "left" | "right"
-}) {
+} & Omit<ComponentPropsWithoutRef<"button"> & { asChild?: boolean }, "children">) {
return (
- <Button variant={variant} size="lg">
+ <Button variant={variant} size="lg" {...props}>
{iconPosition === "left" && <Icon />}
{text}
{iconPosition === "right" && <Icon />}
</Button>
)
}This allows usage like:
<ButtonWithIcon icon={Users} text="Click me" onClick={handleClick} />
<ButtonWithIcon icon={Users} text="Go to page" asChild>
<Link href="/page" />
</ButtonWithIcon>🤖 Prompt for AI Agents
In src/components/ui/buttonWithIcon.tsx around lines 4 to 22, the ButtonWithIcon
component doesn't accept or forward interactive/HTML button props (onClick,
href, asChild, etc.), so update its props to extend the underlying Button's
props (e.g., ButtonProps or React.ButtonHTMLAttributes<HTMLButtonElement> and
support an optional asChild flag and children), allow passing through rest
props, render children when provided (for asChild usage), and spread the
remaining props onto the rendered Button element so consumers can pass onClick,
href via asChild, disabled, aria-attributes, and other common attributes.
Closes #12