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

fix: Update anyOf schema to correctly update items in an array #1491

Conversation

SuriGill
Copy link
Contributor

@SuriGill SuriGill commented Oct 21, 2019

Reasons for making this change

componentWillReceiveProps is no longer recommended by React after updating to React 16 as it has some design flaws. Instead, I've updated to a componentDidUpdate that will change after formData changes specifically. I've added a test to demonstrate the bug that was causing an update twice and selectedOption to become out of sync when moving indices in an array using anyOf.

If this is related to existing tickets, include links to them as well.

fixes first issue in #1486

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Thanks! Might it have been better to use getDerivedStateFromProps instead though? I won't block merging the PR on this, though, as this is clearly an improvement from the previous code.

if (matchingOption === this.state.selectedOption) {
return;
}
if (!prevState || matchingOption === this.state.selectedOption) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add the !prevState check @SuriGill ? It seems like the tests already pass without it.

@epicfaace epicfaace merged commit 29c704e into rjsf-team:master Nov 9, 2019
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

Successfully merging this pull request may close these issues.

2 participants