-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Storybook & shadcn setup #65
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emestabillo Please review my comments.
components/CustomButton.tsx
Outdated
label, | ||
icon: IconComponent, | ||
className, | ||
...props |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this wildcard prop. Let's be explicit with what props we are allowing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wildcard prop removed
components/CustomButton.tsx
Outdated
import { LucideProps } from "lucide-react"; | ||
|
||
interface ButtonProps { | ||
variant?: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all optional, which is not ideal. Please require the props that we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would we handle the different variants?
a. button with text
b. button with icon
c. button with icon and text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only be building components from the design.
} | ||
|
||
const Button = React.forwardRef<HTMLButtonElement, ButtonProps>( | ||
({ className, variant, size, asChild = false, ...props }, ref) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, let's be explicit with our props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component is from Shadcn. Make adjustments?
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Close to clean up in future. |
Description
Closes #53 , Closes #48
Type of change