fix: improve Button component accessibility and reusability - Add typ… #42 - #44
Open
ankitrraj wants to merge 1 commit into
Open
fix: improve Button component accessibility and reusability - Add typ… #42#44ankitrraj wants to merge 1 commit into
ankitrraj wants to merge 1 commit into
Conversation
…e=button as default, accept all HTML button props, add TypeScript interface, add hover/active/disabled states - Fixes DjedAlliance#42
WalkthroughThe Button component was refactored to improve type safety and prop handling. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Author
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/Button.tsx (1)
15-30: Accessibility + reusability: addfocus-visiblestyles and mergestyleinstead of lettingprops.stylewipe defaults.
Right now, passingstylevia props will override (and remove) the default boxShadow because...propscomes afterstyle.export function Button({ children, className, variant = 'primary', type = 'button', + style, ...props }: ButtonProps) { return ( <button type={type} className={twMerge( - 'relative py-1.5 px-2.5 sm:py-2 sm:px-3 rounded-lg font-medium text-xs sm:text-sm bg-gradient-to-b from-[#331500] to-[#FF863B] transition-transform hover:scale-105 active:scale-95 disabled:opacity-50 disabled:cursor-not-allowed disabled:hover:scale-100', + 'relative py-1.5 px-2.5 sm:py-2 sm:px-3 rounded-lg font-medium text-xs sm:text-sm bg-gradient-to-b from-[#331500] to-[#FF863B] transition-transform hover:scale-105 active:scale-95 disabled:opacity-50 disabled:cursor-not-allowed disabled:hover:scale-100 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-[#FF863B]/70 focus-visible:ring-offset-2 focus-visible:ring-offset-black/20', className )} - style={{ boxShadow: '0 0 12px #FF863B' }} + style={{ boxShadow: '0 0 12px #FF863B', ...style }} {...props} >
Comment on lines
+1
to
+5
| import { twMerge } from 'tailwind-merge' | ||
|
|
||
| interface ButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { | ||
| variant?: 'primary' | 'secondary' | ||
| } |
There was a problem hiding this comment.
Fix missing React type import (current code won’t typecheck/compile).
You reference React.ButtonHTMLAttributes but don’t import React (type). Import it, or stop using the React. namespace.
+import type * as React from 'react'
import { twMerge } from 'tailwind-merge'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { twMerge } from 'tailwind-merge' | |
| interface ButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { | |
| variant?: 'primary' | 'secondary' | |
| } | |
| import type * as React from 'react' | |
| import { twMerge } from 'tailwind-merge' | |
| interface ButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { | |
| variant?: 'primary' | 'secondary' | |
| } |
🤖 Prompt for AI Agents
In src/components/Button.tsx around lines 1-5, the component uses
React.ButtonHTMLAttributes but React is not imported causing typecheck failures;
fix by either adding an import for React types (e.g., `import type React from
'react'` or `import React from 'react'` depending on tsconfig) or switch to the
direct type import `import type { ButtonHTMLAttributes } from 'react'` and
update the interface to extend ButtonHTMLAttributes<HTMLButtonElement>; ensure
imports use `import type` when only types are needed to avoid runtime imports.
Comment on lines
+3
to
+13
| interface ButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { | ||
| variant?: 'primary' | 'secondary' | ||
| } | ||
|
|
||
| export function Button({ | ||
| children, | ||
| className, | ||
| variant = 'primary', | ||
| type = 'button', | ||
| ...props | ||
| }: ButtonProps) { |
There was a problem hiding this comment.
variant is part of the public API but currently does nothing.
Either wire it into the className (recommended) or remove it to avoid a misleading prop.
Example wiring:
export function Button({
children,
className,
variant = 'primary',
type = 'button',
...props
}: ButtonProps) {
+ const variantClasses =
+ variant === 'secondary'
+ ? 'from-slate-700 to-slate-900'
+ : 'from-[#331500] to-[#FF863B]'
return (
<button
type={type}
className={twMerge(
- 'relative py-1.5 px-2.5 sm:py-2 sm:px-3 rounded-lg font-medium text-xs sm:text-sm bg-gradient-to-b from-[#331500] to-[#FF863B] transition-transform hover:scale-105 active:scale-95 disabled:opacity-50 disabled:cursor-not-allowed disabled:hover:scale-100',
+ 'relative py-1.5 px-2.5 sm:py-2 sm:px-3 rounded-lg font-medium text-xs sm:text-sm bg-gradient-to-b transition-transform hover:scale-105 active:scale-95 disabled:opacity-50 disabled:cursor-not-allowed disabled:hover:scale-100',
+ variantClasses,
className
)}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Button.tsx around lines 3 to 13, the exported Button accepts a
variant prop but never uses it; either remove the prop or apply it to the
rendered element. Fix by mapping variant to CSS classes (e.g. determine a base
className and append variant-specific classes for 'primary' and 'secondary'),
merge with any incoming className, and pass the combined className to the button
element; keep default variant='primary' and preserve other props.
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.
…e=button as default, accept all HTML button props, add TypeScript interface, add hover/active/disabled states - Fixes #42
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.