-
Notifications
You must be signed in to change notification settings - Fork 120
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(821): Rename size to iconSize in CloseButton #852
Conversation
- Rename the prop and in the component and for the type as well - Update snapshots
Codecov Report
@@ Coverage Diff @@
## master #852 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 52 52
Lines 521 521
=========================================
Hits 521 521
Continue to review full report at Codecov.
|
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 know it sucks but this would be a breaking change, so we should hold off on this for now 😞
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.
Was going to say the same thing as @James300 😞
title: 'close', | ||
} | ||
|
||
CloseButton.propTypes = { | ||
onClick: PropTypes.func, | ||
size: PropTypes.number, | ||
iconSize: PropTypes.number, |
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.
For consistency, I think it'd make sense to keep size
, but change it to use the same values as size
on Button
. Then, we could add iconSize
, which would control the size of the icon. However, if we remove size
or change it so it no longer accepts a number, I think that would be a breaking change.
Closing as we need a better, less break-y way to do this. |
size
prop toiconSize
to prevent type conflicts betweenButton
andCloseButton
Closes #821