Skip to content
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

feat(PF Checkbox): Add 3rd state to checkbox controlled by consumer #2252

Merged
merged 2 commits into from Jun 27, 2019

Conversation

@karelhala
Copy link
Contributor

karelhala commented Jun 13, 2019

What:
When using checkbox as hierarchical checkbox (parent checkbox with child checkboxes) and user selects only some of child checkboxes parent one should go to 3rd state to indicate user that only some checkboxes were selected.

Additional issues:

@karelhala karelhala requested review from jschuler, dlabaj, tlabaj and redallen Jun 13, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 13, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 13, 2019

Codecov Report

Merging #2252 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2252      +/-   ##
==========================================
- Coverage   79.89%   79.87%   -0.02%     
==========================================
  Files         669      669              
  Lines        8529     8531       +2     
  Branches      734      735       +1     
==========================================
  Hits         6814     6814              
- Misses       1362     1363       +1     
- Partials      353      354       +1
Flag Coverage Δ
#patternfly3 85.23% <ø> (ø) ⬆️
#patternfly4 74.88% <0%> (-0.04%) ⬇️
#patternflymisc 95.79% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...-4/react-core/src/components/Checkbox/Checkbox.tsx 85.29% <0%> (-5.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91382a1...7c6438a. Read the comment docs.

@karelhala karelhala force-pushed the karelhala:tri-state-checkbox branch from beda171 to 14b36aa Jun 13, 2019
@rachael-phillips

This comment has been minimized.

Copy link
Contributor

rachael-phillips commented Jun 13, 2019

@mcarrano @tlabaj have we introduced this in Core? If this is a need we should also get ahead of this in Core.

@karelhala

This comment has been minimized.

Copy link
Contributor Author

karelhala commented Jun 13, 2019

@rachael-philips this is just a JS update no need to change or anything in core it uses indeterminate API od checkbox [1].

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/:indeterminate

@rachael-phillips

This comment has been minimized.

Copy link
Contributor

rachael-phillips commented Jun 13, 2019

Thanks for the explanation @karelhala !

@karelhala karelhala mentioned this pull request Jun 13, 2019
@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Jun 13, 2019

I am fine to introduce this directly into React. We may decide to revisit this later based on the discussion here: patternfly/patternfly-next#1411

@@ -46,6 +47,14 @@ class ControlledCheckbox extends React.Component {
id="check-2"
name="check2"
/>
<Checkbox

This comment has been minimized.

Copy link
@jschuler

jschuler Jun 13, 2019

Collaborator

would be good to wrap 2 checkboxes inside the checkbox and show the indeterminate state live. And can also get rid of several other duplicate examples

This comment has been minimized.

Copy link
@karelhala

karelhala Jun 13, 2019

Author Contributor

Yeah! That's a good point. I like it!

@karelhala karelhala force-pushed the karelhala:tri-state-checkbox branch from 5a43de8 to 7c6438a Jun 14, 2019
@rachael-phillips rachael-phillips added this to the Bluebird milestone Jun 14, 2019
@@ -77,6 +79,7 @@ export class Checkbox extends React.Component<CheckboxProps> {
aria-invalid={!isValid}
aria-label={ariaLabel}
disabled={isDisabled}
ref={elem => elem && (elem.indeterminate = isChecked === null)}

This comment has been minimized.

Copy link
@redallen

redallen Jun 14, 2019

Contributor

What's this ref needed for?

This comment has been minimized.

Copy link
@karelhala

karelhala Jun 14, 2019

Author Contributor

This prop is can't be set trough react prop for some reason [1]. However plain js api allows to set it trough element, so this ref is to access checkox element. It's probably because indeterminate is a flag and not real prop though.

[1] facebook/react#1798

@dlabaj
dlabaj approved these changes Jun 27, 2019
@dlabaj dlabaj merged commit cf06254 into patternfly:master Jun 27, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rachael-phillips rachael-phillips modified the milestones: Bluebird, Albatross Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.