-
Notifications
You must be signed in to change notification settings - Fork 350
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): Set checked value correctly #1929
Conversation
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
PatternFly-React preview: https://1929-pr-patternfly-react-patternfly.surge.sh |
Codecov Report
@@ Coverage Diff @@
## master #1929 +/- ##
==========================================
+ Coverage 82.63% 82.64% +0.01%
==========================================
Files 624 624
Lines 6875 6879 +4
Branches 93 94 +1
==========================================
+ Hits 5681 5685 +4
Misses 1154 1154
Partials 40 40
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.
@boaz0 can you add issue number to the description
Done 👍 |
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.
LGTM
What:
closes #1959
I first realized that the controlled Checkbox example is wrong:
event.target.name
is not set and so the state can be{check1: false, check2: false, '': false}
or{check1: false, check2: false, '': true}
.After fixing it by adding the
name
attribute to the Checkbox components the first time I checked one of the boxes I got the following error/warning message:It looks like the expression
isChecked || checked
will return undefined ifisChecked
is false and so when checking the box that expression changed totrue
which causes the component to turn to be controlled.//cc @dlabrecq
fixes https://github.com/project-koku/koku-ui/issues/841