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

Added nextState to Field/FieldArray shouldComponentUpdate #1171

Merged
merged 6 commits into from
Jun 17, 2016
Merged

Conversation

erikras
Copy link
Member

@erikras erikras commented Jun 16, 2016

The shouldComponentUpdate needs both nextState and nextContext to know whether or not it needs to update. Ignoring nextState was previously causing issues with shallowCompare, because this.state was null and nextState was undefined (not passed).

The sync error also needs to be pulled from nextContext to know whether or not it needs to update.

This was originally raised in #1141.

@ooflorent, please review. (ignore the test in FieldArray.spec.js)

@erikras erikras added this to the next-6.0.0 milestone Jun 16, 2016
@erikras
Copy link
Member Author

erikras commented Jun 16, 2016

@Darmody, how's this?

@ooflorent
Copy link
Contributor

Why is the implementation of getSyncError different from both components?
Otherwise LGTM

@erikras
Copy link
Member Author

erikras commented Jun 16, 2016

No good reason. I'll sync them.

@erikras
Copy link
Member Author

erikras commented Jun 17, 2016

This is me using git. 🙈

d6a1143f571184db25f94613edd43b40af6d3a629221aba00d9efdcfef5efd84

I was trying to squash commits and made a mess of things.

@erikras erikras merged commit 0684782 into v6 Jun 17, 2016
@erikras
Copy link
Member Author

erikras commented Jun 17, 2016

No good reason.

I was wrong. There is a good reason, and I have documented it with a comment.

@Darmody
Copy link

Darmody commented Jun 18, 2016

Hmm, @erikras, LGTM. I will try it in my project, and give you my feedback.

@erikras
Copy link
Member Author

erikras commented Jul 1, 2016

Published in v6.0.0-rc.1.

@erikras erikras deleted the nextState branch July 28, 2016 15:00
@lock
Copy link

lock bot commented Jun 2, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2018
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.

None yet

3 participants