Skip to content
This repository was archived by the owner on Jan 14, 2025. It is now read-only.

Provide controlled Checkbox #552

Closed
levibotelho opened this issue Dec 19, 2018 · 6 comments
Closed

Provide controlled Checkbox #552

levibotelho opened this issue Dec 19, 2018 · 6 comments

Comments

@levibotelho
Copy link

It should be possible to use Checkbox as a controlled component. Currently it takes a checked prop, but clicking on the checkbox changes the value even if checked isn't updated.

I suggest changing checked to defaultChecked, and adding a new checked property that when set makes the Checkbox act like a controlled component.

I'm happy to do the work for this change.

@moog16
Copy link

moog16 commented Dec 19, 2018

Is this a dup of #438?

@levibotelho
Copy link
Author

levibotelho commented Jan 2, 2019

I don't understand the core issue behind #438. Is it about making the React code more semantically similar to the HTML output? Is there a record of the problem from the Discord chat somewhere?

@moog16
Copy link

moog16 commented Jan 2, 2019

#438 provides access to the <input type='checkbox'> element. This way you should be able to control the checkbox with regular html attributes.

Regarding your original request. The checkbox component does have that functionality. From lines 84 to 95:

  componentDidUpdate(prevProps: CheckboxProps) {
    const {checked, indeterminate, disabled} = this.props;
    if (
      checked !== prevProps.checked ||
      indeterminate !== prevProps.indeterminate
    ) {
      this.handleChange(checked!, indeterminate!);
    }
    if (disabled !== prevProps.disabled) {
      this.foundation.setDisabled(disabled);
    }
  }

@levibotelho
Copy link
Author

The problem with the current API is that the component is halfway between a controlled and uncontrolled component. If you set the value the checkbox updates, but if the user clicks the checkbox and you don't update the value then the checkbox's state falls out of sync with the value prop which can be problematic. Furthermore it's not possible to prevent the checkbox's value from changing by listening to onChange and not updating the value prop.

React handles this elegantly by providing checked and defaultChecked props. If you set checked to a value there's no way to change the checkbox other than by listening to onChange and updating that value. Just clicking the checkbox won't do anything without the event handler, so checked always stays in sync with the component's state in the UI.

If instead you set defaultValue, then that value is set at the beginning of the checkbox's life and has no further impact. You can't use defaultValue to change the state of the checkbox once it's been loaded, and you aren't required to update anything in your event handler (or to even have one) to allow the checkbox to change state when a user clicks it.

I've created a small demo here: http://jsfiddle.net/ytq61r8a/1/.

What I'd like to do is provide users with the same API as they get when using a vanilla React <input type="checkbox" />.

@moog16
Copy link

moog16 commented Feb 21, 2019

@levibotelho, after reading your last post it sounds like the proposed fix of #438 would also fix the issue you're encountering. The idea is to allow people to compose the checkbox like so:

<Checkbox>
  <NativeCheckbox {...inputProps} />
</Checkbox>

With this change, you should be able to pass whatever props you'd like.

@levibotelho
Copy link
Author

@moog16 makes sense, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants