Skip to content

Conversation

@jaulz
Copy link
Collaborator

@jaulz jaulz commented Jul 6, 2018

Motivation

Implement indeterminate Checkboxes according to:
https://material.io/design/components/selection-controls.html#checkboxes

Test plan

image

@callstack-bot
Copy link

callstack-bot commented Jul 6, 2018

Hey @jaulz, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Thanks for the PR. I think instead of having 2 props, having a single prop would be better, because checked and indeterminate don't mean anything together because indeterminate means that the state is unknown.

status: 'checked' | 'unchecked' | 'indeterminate'

We should do the same for radio :)

@jaulz
Copy link
Collaborator Author

jaulz commented Jul 6, 2018

Hm, I'm not sure if that's a good idea from a developer perspective since he needs to handle strings instead of truthy/falsey values.

@satya164
Copy link
Member

satya164 commented Jul 6, 2018

It's a minor inconvenience, but better than an inconsistent API IMO. Having two separate props which control the same thing also means that people who actually use the indeterminate will have to deal with 2 props in all places which can be cumbersome, and if someone wants to add this behavior to existing app, it'll require more refactoring.

It's also easier to introduce bugs when 2 props control the same state.

@jaulz
Copy link
Collaborator Author

jaulz commented Jul 9, 2018

@satya164 just implemented the changes as suggested :) The indeterminate status for radio buttons is by the way not part of the specs (https://material.io/design/components/selection-controls.html#radio-buttons).

@ferrannp
Copy link
Collaborator

@satya164 can you re-check?

@jaulz
Copy link
Collaborator Author

jaulz commented Aug 27, 2018

@satya164 any chance to have a look at it for the 2.0 release?

@satya164
Copy link
Member

merged in 3b09171

@satya164 satya164 closed this Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants