feat(component): create button component/variants#70
Conversation
aleckshen
left a comment
aleckshen
left a comment
There was a problem hiding this comment.
small minor changes, you the goat man, good PR
|
|
||
| const buttonVariants = cva( | ||
| "group/button inline-flex shrink-0 items-center justify-center rounded-lg border border-transparent bg-clip-padding text-sm font-medium whitespace-nowrap transition-all outline-none select-none focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 active:not-aria-[haspopup]:translate-y-px disabled:pointer-events-none disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4", | ||
| "group/button inline-flex shrink-0 items-center justify-center rounded-lg border-3 border-transparent bg-clip-padding text-xl font-body whitespace-nowrap transition-all outline-none select-none focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 active:not-aria-[haspopup]:translate-y-px disabled:pointer-events-none disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4 transition-colors duration-300 ease-out", |
There was a problem hiding this comment.
border-3 isn't a Tailwind class. Tailwind ships border (1px), border-2, border-4, border-8. This is silently a no-op, so the border falls back to whatever border (1px) would give if it were there, or nothing.
This matters because the primary and cta variants rely on the border becoming visible on hover (default has bg-brand-primary + border-brand-primary, hover swaps to bg-transparent so the border should show through as an outline). With border-3 not applying, the hover state will look like floating text instead of an outlined button.
Can you change this to border-2 (or border-4 depending on what matches Figma)?
|
|
||
| const buttonVariants = cva( | ||
| "group/button inline-flex shrink-0 items-center justify-center rounded-lg border border-transparent bg-clip-padding text-sm font-medium whitespace-nowrap transition-all outline-none select-none focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 active:not-aria-[haspopup]:translate-y-px disabled:pointer-events-none disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4", | ||
| "group/button inline-flex shrink-0 items-center justify-center rounded-lg border-3 border-transparent bg-clip-padding text-xl font-body whitespace-nowrap transition-all outline-none select-none focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 active:not-aria-[haspopup]:translate-y-px disabled:pointer-events-none disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4 transition-colors duration-300 ease-out", |
There was a problem hiding this comment.
There are two conflicting transition declarations on this line — transition-all near the start and transition-colors duration-300 ease-out at the end. transition-colors wins and overrides transition-all, which means the active:…translate-y-px press-down effect (also on this line) won't animate smoothly because transform isn't in the transitioned property list.
Can you consolidate to a single declaration that covers both, e.g. transition-[colors,transform] duration-300 ease-out?
|
|
||
| const buttonVariants = cva( | ||
| "group/button inline-flex shrink-0 items-center justify-center rounded-lg border border-transparent bg-clip-padding text-sm font-medium whitespace-nowrap transition-all outline-none select-none focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 active:not-aria-[haspopup]:translate-y-px disabled:pointer-events-none disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4", | ||
| "group/button inline-flex shrink-0 items-center justify-center rounded-lg border-3 border-transparent bg-clip-padding text-xl font-body whitespace-nowrap transition-all outline-none select-none focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 active:not-aria-[haspopup]:translate-y-px disabled:pointer-events-none disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4 transition-colors duration-300 ease-out", |
There was a problem hiding this comment.
Could you add cursor-pointer to the base classes? Default <button> cursor in some browsers is cursor: default, so right now it doesn't clearly indicate clickability on hover. We want the hand cursor on hover to signal it's interactive.
2 participants
Description
This PR creates a reusable button component which can be used across all pages, and matches the UOAVC design in Figma.
Closes #52
Type of change
How Has This Been Tested?
Checklist before requesting a review