Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improved the "Button" UI component #61

Merged
merged 8 commits into from
Dec 8, 2023
Merged

Improved the "Button" UI component #61

merged 8 commits into from
Dec 8, 2023

Conversation

nick-y-ito
Copy link
Member

@nick-y-ito nick-y-ito commented Dec 6, 2023 •
edited
Loading

Overview

Improved the src/components/ui/Button.tsx file to match it with the Figma design since it was nearly the default state generated by shadcn/ui.

Changes

  • Defined three types of variants (primary/error/cancel)
  • Defined three sizes, adjusting to make the buttons' sizes as close as possible to the Figma design.
  • Capitalized the file name to make it conventional.

Note

@nick-y-ito nick-y-ito self-assigned this Dec 6, 2023
@nick-y-ito nick-y-ito linked an issue Dec 6, 2023 that may be closed by this pull request
Create "Button" shared UI components #56
Closed
@vercel Vercel
Copy link

vercel bot commented Dec 6, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nishiki-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 6, 2023 9:23pm

@kanta1207
Copy link
Member

Great job! Button component is refined much better now!
Please let me hear your opinion on adding "icon" variant to Button component,
and except for that, I think we can merge this change!

@nick-y-ito
Copy link
Member Author

@kanta1207
Thank you for your prompt review! It would be better to create another component solely for icons.

@nick-y-ito
Copy link
Member Author

@kanta1207 I have resolved the conflict. Please review it again.

kanta1207
kanta1207 approved these changes Dec 8, 2023
View reviewed changes
Copy link
Member

@kanta1207 kanta1207 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response, didn't reallize there was conflict.
LGTM, great job!

src/components/base/parts/mobile/MobileHeader.tsx Outdated
children: React.ReactNode;
}

const Button: React.FC<IButtonProps> = ({ children, ...props }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Button component for Icons looks great, and reusabale to me!
What do you think of the idea of adding this classname to button component, as variant "icon" ?

Copy link
Member Author

@nick-y-ito nick-y-ito Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea if this component will be re-used by being called from other files. In this case, however, the Button component is private, meaning there is no export keyword on it; thus, I think className is unnecessary for now. We can add it when it's needed. (YAGNI)

@nick-y-ito nick-y-ito merged commit fb4f280 into develop Dec 8, 2023
4 checks passed
@nick-y-ito nick-y-ito deleted the feat/56-button-component branch December 8, 2023 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kanta1207 kanta1207 kanta1207 approved these changes

@kotaaaa kotaaaa Awaiting requested review from kotaaaa

Assignees

@nick-y-ito nick-y-ito

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create "Button" shared UI components
2 participants
@nick-y-ito @kanta1207