-
Notifications
You must be signed in to change notification settings - Fork 4k
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(Dropdown|Label): Remove overzealous prop validations #828
Conversation
Current coverage is 100% (diff: 100%)
|
31ffe9c
to
6a7abaf
Compare
@levithomason - this fixes an annoying/incorrect prop validation warning in Aside from the |
Hm, I read up on this but I think this fix should be to the default and not the validation. My reasoning is that the I would think then that the issue is that there should not be a default, since having one would break certain use cases. Let me know your thoughts on this. |
a9524cb
to
cdc9f85
Compare
.should.not.have.descendants('Icon') | ||
}) | ||
|
||
it('has delete icon by default', () => { |
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 believe this test would still apply since there is a default icon, it is just not applied in defaultProps
, true?
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 would think then that the issue is that there should not be a default, since having one would break certain use cases. Let me know your thoughts on this.
As per this comment, I removed the default entirely so you'd have to pass the removeIcon
in addition to onRemove
. That does seem a little redundant though since the 99% use-case would be to have the removeIcon
be delete
. I'll add it back and add the test. I'll just use:
const removeIconShorthand = _.isUndefined(removeIcon) ? 'delete' : removeIcon
cdc9f85
to
de40c7e
Compare
Ok this should be g2g :) |
Released in |
* bug(Dropdown|Label): Remove overzealous prop validations * Remove default removeIcon from Label
Fixes #827