-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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(Checkbox): no onChange/onClick when disabled #601
Conversation
Check if `disabled` before calling onChange
@sprmn Thanks for PR! Your addition failed because you don't spreaded - const { onChange, onClick, name, value } = this.props
+ const { onChange, onClick, disabled, name, value } = this.props Could you add a test-case for this? In it('omitted when has disabled prop', () => {
const click = () => mount(<Checkbox disabled onChange={spy} />).simulate('click')
expect(click).to.not.throw()
}) |
Oops, thought that it was already spread, but that was in the |
You can run tests with |
Current coverage is 100% (diff: 100%)@@ master #601 diff @@
====================================
Files 119 119
Lines 1915 1915
Methods 0 0
Messages 0 0
Branches 0 0
====================================
Hits 1915 1915
Misses 0 0
Partials 0 0
|
A few things:
common.propKeyOnlyToClassName(Checkbox, 'disabled')
common.propKeyOnlyToClassName(Checkbox, 'readOnly', {
className: 'read-only',
})
- if (onClick) onClick(e, { name, value, checked: !!checked })
- if (onChange) onChange(e, { name, value, checked: !checked })
-
if (this.canToggle()) {
+ if (onClick) onClick(e, { name, value, checked: !!checked })
+ if (onChange) onChange(e, { name, value, checked: !checked })
+
this.trySetState({ checked: !checked })
} |
Even better! Should be fixed 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.
Checked this out locally and works as expected, 👍 🚢 💨
@levithomason - this is ready to merge when you are 👍 |
Thanks for taking care of this guys! Released in |
Check if
disabled
before callingonChange
function.Fixes #600