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

Checkboxes don't toggle #35

Closed
STRML opened this issue Jan 2, 2016 · 7 comments
Closed

Checkboxes don't toggle #35

STRML opened this issue Jan 2, 2016 · 7 comments

Comments

@STRML
Copy link

STRML commented Jan 2, 2016

It appears the internal switched property (on MUI Checkbox) is no longer being toggled properly when wrapped in a <FormsyCheckbox>.

It appears that:

  • A click calls <EnhancedSwitch>'s _handleChange().
  • This calls this.props.onParentShouldUpdate(), which is implemented by <Checkbox> and calls this.setState({switched: true}).
  • In the same callback, <EnhancedSwitch> also calls this.props.onSwitch(), which is implemented by <Checkbox>.
  • <Checkbox> calls its own onCheck which is set by Formsy-MUI.
  • This calls Formsy-MUI's setValue(), which fires another setState().
  • This causes the <FormsyCheckbox> to rerender, which fires <Checkbox>'s componentWillReceiveProps.
  • <Checkbox>'s componentWillReceiveProps calls its own setState(), which looks at the value of this.state.switched and overwrites it. But it hasn't changed yet, the transition is still pending, so it is always set to false.

Not sure what the fix is here. <EnhancedSwitch> should probably be passing a callback to onParentShouldUpdate() that it can send to setState(), to ensure the state transition flushes before any other handlers fire.

@STRML
Copy link
Author

STRML commented Jan 2, 2016

Here's a photo showing <Checkbox> handling the cWRP callback, while the switched: true transition is still pending:

@mbrookes
Copy link
Collaborator

mbrookes commented Jan 4, 2016

@STRML - thanks for the detailed report. Looks like something changed between 0.14.0-rc1 and 0.14.0-rc2. @alitaheri any thoughts?

@alitaheri
Copy link

We're planning to make all our components controllable with stateful wrappers to help with forms. There are indeed many problems regarding how state is handled. and we should not handle state at all! It's the user's concern how the data flows through their application. we shouldn't make assumptions! @STRML Whenever you get the time please open an issue in material-ui repository too. We should track these bugs Thanks 😁... oh sorry I meant ( ^_^ )

@mbrookes
Copy link
Collaborator

mbrookes commented Jan 4, 2016

Thanks 😁... oh sorry I meant ( ^_^ )

😀

@STRML
Copy link
Author

STRML commented Jan 4, 2016

Great, thanks!

@mbrookes
Copy link
Collaborator

mbrookes commented Jan 4, 2016

No worries. Can you test against master, and if all looks good for you, I'll push a new release.

@STRML
Copy link
Author

STRML commented Jan 4, 2016

👍 Works here.

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

No branches or pull requests

3 participants