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

Add type to buttons #349

Merged
merged 7 commits into from
Dec 19, 2018
Merged

Add type to buttons #349

merged 7 commits into from
Dec 19, 2018

Conversation

dferber90
Copy link
Contributor

@dferber90 dferber90 commented Dec 18, 2018

This PR adds the type prop to multiple buttons (FlatButton, IconButton, PrimaryButton, SecondaryButton, SecondaryIconButton) to enable using them as submit/reset buttons in forms.

However the FlatButton already had a prop called type which only supported primary or secondary as values. That prop was renamed to tone. The naming is not ideal but good enough until we redo the buttons. The PrimaryButton also uses a tone prop at the moment. Note that the usage of tone is a temporary change and is supposed to be redone once the buttons get reorganised.

I further did not add a type prop to the LinkButton as it's not really a button in the first place. It always renders a link. We should have a Link category and this should probably be a ButtonLink (as it looks like a button but is a link).

Closes #340

BREAKING CHANGE: The type prop of FlatButton was renamed to tone. A new type prop was added to FlatButton instead for which possible values are submit, reset and button.

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

❤️ 🎉

Copy link
Contributor

@montezume montezume left a comment

Choose a reason for hiding this comment

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

Can you remove the percy tests?

@dferber90
Copy link
Contributor Author

Thanks for fixing things @montezume. Some tests broke due to the added warning (because we were passing type and linkTo in our tests). I pushed another commit removing the type in those cases to avoid the warning. All working again 👍

@@ -343,7 +343,7 @@ describe('interaction', () => {
let wrapper;
describe('when not disabled', () => {
beforeEach(() => {
props = createProps({ linkTo: '/foo/bar' });
props = createProps({ type: undefined, linkTo: '/foo/bar' });
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Contributor

@montezume montezume left a comment

Choose a reason for hiding this comment

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

Ready when you are 🚀

@dferber90 dferber90 force-pushed the df-add-type-to-buttons branch from 4870c81 to 622ba65 Compare December 19, 2018 13:34
@dferber90 dferber90 merged commit f82e324 into master Dec 19, 2018
@dferber90 dferber90 deleted the df-add-type-to-buttons branch December 19, 2018 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💅 Type: Enhancement Improves existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants