-
Notifications
You must be signed in to change notification settings - Fork 3
Add a reusable <Button/> component #274
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
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for criipto-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
To further align the documentation with the design system.
cedd189 to
a40798b
Compare
Trinurt
left a comment
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.
I like it very much 👍
src/components/Button/Button.tsx
Outdated
| const variants = { | ||
| primary: cx('bg-primary-600 text-white', !isDisabled && 'hover:bg-primary-800'), | ||
| default: cx( | ||
| 'border border-light-blue-700/30 text-light-blue-800 bg-transparent', | ||
| !isDisabled && 'hover:bg-primary-600/10', | ||
| ), | ||
| dark: cx( | ||
| 'bg-transparent border border-white/40 text-white', | ||
| !isDisabled && 'hover:bg-white/20', | ||
| ), | ||
| }; | ||
|
|
||
| const sizes = { | ||
| sm: 'h-8 leading-4', | ||
| md: 'h-10 leading-4', | ||
| lg: 'h-12 leading-5 text-base', | ||
| }; |
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.
For extra type-safety and to produce more actionable error messages when we add new variants, I would propose adding an explicit type annotation to these definitions
| const variants = { | |
| primary: cx('bg-primary-600 text-white', !isDisabled && 'hover:bg-primary-800'), | |
| default: cx( | |
| 'border border-light-blue-700/30 text-light-blue-800 bg-transparent', | |
| !isDisabled && 'hover:bg-primary-600/10', | |
| ), | |
| dark: cx( | |
| 'bg-transparent border border-white/40 text-white', | |
| !isDisabled && 'hover:bg-white/20', | |
| ), | |
| }; | |
| const sizes = { | |
| sm: 'h-8 leading-4', | |
| md: 'h-10 leading-4', | |
| lg: 'h-12 leading-5 text-base', | |
| }; | |
| const variants: Record<BaseButtonProps['variant'], string> = { | |
| primary: cx('bg-primary-600 text-white', !isDisabled && 'hover:bg-primary-800'), | |
| default: cx( | |
| 'border border-light-blue-700/30 text-light-blue-800 bg-transparent', | |
| !isDisabled && 'hover:bg-primary-600/10', | |
| ), | |
| dark: cx( | |
| 'bg-transparent border border-white/40 text-white', | |
| !isDisabled && 'hover:bg-white/20', | |
| ), | |
| }; | |
| const sizes: Record<BaseButtonProps['size'], string> = { | |
| sm: 'h-8 leading-4', | |
| md: 'h-10 leading-4', | |
| lg: 'h-12 leading-5 text-base', | |
| }; |
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.
src/components/Button/Button.tsx
Outdated
| const variants = { | ||
| primary: cx('bg-primary-600 text-white', !isDisabled && 'hover:bg-primary-800'), | ||
| default: cx( | ||
| 'border border-light-blue-700/30 text-light-blue-800 bg-transparent', | ||
| !isDisabled && 'hover:bg-primary-600/10', | ||
| ), | ||
| dark: cx( | ||
| 'bg-transparent border border-white/40 text-white', | ||
| !isDisabled && 'hover:bg-white/20', | ||
| ), | ||
| }; |
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 might be personal preference, but since you're using classNames, I would propose using the object syntax to define the conditional classes instead, since it makes it much easier to add multiple cases in the future.
| const variants = { | |
| primary: cx('bg-primary-600 text-white', !isDisabled && 'hover:bg-primary-800'), | |
| default: cx( | |
| 'border border-light-blue-700/30 text-light-blue-800 bg-transparent', | |
| !isDisabled && 'hover:bg-primary-600/10', | |
| ), | |
| dark: cx( | |
| 'bg-transparent border border-white/40 text-white', | |
| !isDisabled && 'hover:bg-white/20', | |
| ), | |
| }; | |
| const variants = { | |
| primary: cx('bg-primary-600 text-white', { 'hover:bg-primary-800': !isDisabled }), | |
| default: cx( | |
| 'border border-light-blue-700/30 text-light-blue-800 bg-transparent', | |
| { 'hover:bg-primary-600/10': !isDisabled }, | |
| ), | |
| dark: cx( | |
| 'bg-transparent border border-white/40 text-white', | |
| { 'hover:bg-white/20': !isDisabled }, | |
| ), | |
| }; |
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.
src/components/Button/Button.tsx
Outdated
| return cx(base, variants[props.variant], sizes[size], iconPadding, isDisabled && 'opacity-40'); | ||
| } | ||
|
|
||
| function Inner(props: BaseButtonProps & (TextButtonProps | IconButtonProps)) { |
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.
Proposing renaming this to ButtonContent to better describe the component
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.
src/components/Button/Button.tsx
Outdated
| extends React.ButtonHTMLAttributes<HTMLButtonElement>, | ||
| BaseButtonProps {} | ||
|
|
||
| export default function Button({ |
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.
Propose removing the default from the export, since it improves auto-complete discoverability and consistency in imports
See more: https://dev.to/phuocng/avoid-using-default-exports-a1c
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.
I had no idea! Thanks! 2fb60bc
src/components/Header.tsx
Outdated
| <Button variant="primary" size="sm" className="uppercase"> | ||
| <a href="https://dashboard.idura.app/signup?utm_source=docs" target="_blank"> | ||
| Sign up | ||
| </a> | ||
| </Button> |
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 might be a potential use for the AnchorButton?
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.
Fixed in 2fb60bc
To further align the documentation with the design system.
QA: Please check the updated buttons:
button: https://deploy-preview-274--criipto-docs.netlify.app/signatures/webhooks/tester/ (available after you input API credentials, or see screenshot).