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: refactor button to use styled-components #145

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore: refactor button to use styled-components #145

wants to merge 1 commit into from

Conversation

pixelass
Copy link

@pixelass pixelass commented Sep 11, 2019

This is just a suggestion @gabrielbull.

It does not implement the basic integration of the library features.

Right now I just removed all other dependencies/features. I kept the version for the styles to allow supporting different os x versions.

Screen Shot 2019-09-11 at 14 35 48

does not implement basic integration
@davej
Copy link
Collaborator

davej commented Sep 11, 2019

I think we should limit the changes to styled-components only and not change the logic or introduce hooks.

I would be in favour of those changes too but not in this PR.

@pixelass
Copy link
Author

I fully understand. The Button component would only wrap the StyledButton and radium would be removed.

The main reason for the bigger change was that I'm not sure how the old customization would play with the new styling. I think most things could be handled over a ThemeProvider and could therefore be simplified.

The only things that would stay are the eventHandling (onEnter, onClick...) which I would have implemented in hooks for simplicity. I can make the example more complete here and open a new draft where I try to keep the underlying logic (while I think it might be messy in favor of keeping changes to a minimum).

IMHO the change would require a major bump anyways.

That said, I haven't really looked into this lib yet so these are just assumptions.

Thx for the feedback so far.

@davej
Copy link
Collaborator

davej commented Sep 11, 2019

The main reason for the bigger change was that I'm not sure how the old customization would play with the new styling. I think most things could be handled over a ThemeProvider and could therefore be simplified.

Perhaps the best thing to do is get a complete component working with theming? It may be a bit easier to evaluate the pros and cons then.

@langyo langyo marked this pull request as ready for review April 26, 2021 06:41
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