-
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 elements in Alert #221
Conversation
src/Alert/Alert.js
Outdated
@@ -39,11 +41,15 @@ export class Alert extends Component { | |||
aria-controls='j2ALl423' | |||
aria-label='Close' | |||
className='fd-alert__close' | |||
onClick={() => this.closeAlertHandler()} /> | |||
onClick={() => this.closeAlertHandler()} | |||
{...buttonProps} /> |
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 these two cases, 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 🎢
* prop spreading to elements in alert * move spreads * update documentation with new props * Fix dismissible spelling
Adds props for button and link additional props, enables tests