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.

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

Feat: Implement custom button #7

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

Lukas0912
Copy link
Collaborator

You can test the buttons on the /elements page

Copy link
Collaborator

@marinovl7 marinovl7 left a comment

Choose a reason for hiding this comment

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

Left detailed comment about the theming and the coloring of the button component. Generally make use of existing functionalities and features of the libraries we use and do not think about how to implement this alone. Everything will be handled by the libraries for you, thats why we use them, not to define and do everything ourselves!

Also make sure you always merge main into your branch before creating a PR and always check for conflicts!

src/components/Buttons/CustomButton.tsx Outdated Show resolved Hide resolved
src/components/Buttons/CustomButton.tsx Outdated Show resolved Hide resolved
* Custom button component in the variants solid, bordered and flat. It can be used like a normal NextUI button
* component.
* @param variant solid | bordered | flat
* @constructor
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove @constructor

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file contains a lot of duplicated code. Generally all cases could be summarized in one and there is no need of having the same component with few changes for all cases. Imagine having 100 variants for the button, this simply will not work and the file will grow a lot. It's considered a good practice to separate the concerns (here styling and react component).

Nevertheless we use tailwindcss, which means we do not use style on the component but everything will be with className="tailwindClasses". Also you do not need to check for the theme here, neither you do need the useEffects for the theme switch. Everything you need will be handled by NextUI and TailwindCSS. Colors and everything will be defined in thiw way, not custom css colors: Check here. Also check the existing tailwind.config.js and research how to apply all colors and customize them there.

@Lukas0912 Lukas0912 force-pushed the feature/f-42-implement-custom-buttons branch 4 times, most recently from 0e01723 to 0ceb1e5 Compare November 5, 2024 21:17
@Lukas0912
Copy link
Collaborator Author

Since the tailwind classes work properly now I refactored everything and made use of the tailwind classes. The colors when hovering don't align perfectly with Figma because NextUI automatically lowers the opacity when hovered. The alternative would have been to disable this functionality but this could also have an effect on other components.

@Lukas0912 Lukas0912 force-pushed the feature/f-42-implement-custom-buttons branch from 0ceb1e5 to 8ed75e2 Compare November 5, 2024 21:37
@Lukas0912 Lukas0912 force-pushed the feature/f-42-implement-custom-buttons branch from 8ed75e2 to 10e7e99 Compare November 6, 2024 16:51
Copy link
Collaborator

@marinovl7 marinovl7 left a comment

Choose a reason for hiding this comment

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

LGTM!

@marinovl7 marinovl7 merged commit 1b4d6cd into main Nov 8, 2024
1 check passed
@marinovl7 marinovl7 deleted the feature/f-42-implement-custom-buttons branch November 8, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants