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

chore: standardize buttons using a button component #571

Merged
merged 10 commits into from
Nov 26, 2023

Conversation

MarioRodrigues10
Copy link
Member

chore: use a Button component

Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for seium-stg ready!

Name Link
🔨 Latest commit aa1c16c
🔍 Latest deploy log https://app.netlify.com/sites/seium-stg/deploys/65627f3a9bd8820008294562
😎 Deploy Preview https://deploy-preview-571--seium-stg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@diogogmatos
Copy link
Member

pages/register/[uuid].js Outdated Show resolved Hide resolved
Copy link
Contributor

@tiago-bacelar tiago-bacelar left a comment

Choose a reason for hiding this comment

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

I feel like the title field could be a child component. I know 99% of the time it's text, but for the other 1% it would be really useful. Not to mention, it would be less verbose, more intuitive and better follow the composition model of react.

Copy link
Contributor

@Darguima Darguima left a comment

Choose a reason for hiding this comment

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

I would suggest take the ...rest of the Button Props and pass it to the button element, to allow to use the generic HTML button props at the component level, without need future changes or extra complexity.

I think that if you don't want do this, at least you should correct the Props types with the Typescript Pick or Omit operators, because at this moment is correct, at the typescript level, pass any of the HTML buttons props to this component, even if they aren't used.

image

Also, if you want accept the changes I think that would be better the customStyle be the className (already included at ...rest) so as not to create confusion, since they would overwrite the other

components/Button/index.tsx Show resolved Hide resolved
components/Button/index.tsx Show resolved Hide resolved
components/Button/index.tsx Outdated Show resolved Hide resolved
components/Button/index.tsx Outdated Show resolved Hide resolved
Darguima
Darguima previously approved these changes Nov 25, 2023
Copy link
Contributor

@Darguima Darguima left a comment

Choose a reason for hiding this comment

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

Perfect

@MarioRodrigues10 MarioRodrigues10 merged commit c089565 into main Nov 26, 2023
5 checks passed
@MarioRodrigues10 MarioRodrigues10 deleted the mr/make-reusable-button branch November 26, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable and change the color of the buy button when there's not enough tokens
6 participants