-
Notifications
You must be signed in to change notification settings - Fork 77
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
fix: prop spreading to ButtonGroup #222
Conversation
src/Button/Button.js
Outdated
return ( | ||
<div aria-label='Group label' className='fd-button-group' | ||
role='group'> | ||
role='group' | ||
{...props}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want the spread to occur first and then the hard-coded props. Otherwise, consumers could literally override required prop values. In this case, it will also leverage some capabilities of the classnames
story to make sure they can include additional classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging blocked until fundamental-bot admin rights are restored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes look good. 🚢 I can approve once merging is allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's roll 🎢
* enable spreading props to buttongroup * move buttongroup spread
Allows props to be spread to ButtonGroup component