Skip to content
This repository has been archived by the owner on Oct 6, 2020. It is now read-only.

fix(RadioGroup): don't loop state updates #9

Merged
merged 3 commits into from
Dec 3, 2018

Conversation

choochootrain
Copy link

looks like this was the issue here: https://app.asana.com/0/821652619613450/929543341861746/f

componentDidUpdate gets called after handleChange on user input - this was calling handleChange again with the old value (because this.props.value isn't updated until Formbot.setState is called and propagated back down)

@choochootrain choochootrain force-pushed the fix/radiogroup-lifecycle branch from d364adb to 7114cea Compare December 1, 2018 00:37
@codecov-io
Copy link

codecov-io commented Dec 1, 2018

Codecov Report

Merging #9 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #9   +/-   ##
=======================================
  Coverage   69.29%   69.29%           
=======================================
  Files          10       10           
  Lines         127      127           
  Branches       20       20           
=======================================
  Hits           88       88           
  Misses         28       28           
  Partials       11       11

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 ff0f468...853b290. Read the comment docs.

@kylealwyn
Copy link
Contributor

Discussed irl but documenting to also update https://github.com/sappira-inc/molekule/blob/master/src/Form/Checkbox.js#L88 to use getDerivedStateFromProps

@kylealwyn
Copy link
Contributor

kylealwyn commented Dec 1, 2018

@choochootrain I'm also seeing a potential issue with the current logic. If a prop other than value changes, it'll always update the internal state value to the external value, which in this case would be undefined.

Propose logic to look like:

if (props.value && props.value !== state.value) {
  return {
    ...state, // cant remember if this shallow merges?
    value: props.value,
  };
}

return null;

@choochootrain
Copy link
Author

👍 good catch, i'll update. the derived state merges like setState which is nice

* update docs to showcase examples
* fix checkbox color props
@choochootrain choochootrain merged commit b7c9947 into master Dec 3, 2018
@choochootrain choochootrain deleted the fix/radiogroup-lifecycle branch December 3, 2018 18:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants