-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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 local state 'columns' #2896
fix local state 'columns' #2896
Conversation
I found a bug in VisualizeModal: in setStateFromProps(props), we create columns object and set into state. But setStateFromProps will be called every componentWillReceiveProps, which means when user close Modal, open Modal etc, it will reset columns state to original props. As a result if user changed any checkbox, then close and re-open modal, his previous change will lost. This thing will become more annoying when mixed with redux updating its state. I think we should keep 'columns' as a local state, and it should remember states even after user close Modal and reopen it. my fix is just calling setStateFromProps from constructor, not from every componentWillReceiveProps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor semantic issue to fix, otherwise LGTM
this.setStateFromProps(nextProps); | ||
} | ||
setStateFromProps(props) { | ||
setStateFromProps() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably need to change the function name now that it does not setState
anymore. getColumnFromProps
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed method name.
I found a bug in VisualizeModal:
in setStateFromProps(props), we create a columns object and set into state.
But setStateFromProps will be called every componentWillReceiveProps, which means when user close Modal, open Modal etc, it will reset columns state to original props. For example, if user open modal, changed any checkbox, then close and re-open modal, his previous change will lost. This thing will become more annoying when mixed with redux updating its state.
I think we should keep 'columns' as a local state, and it should remember user's changes even after user close Modal and reopen it. My fix is just calling setStateFromProps from constructor, not from every componentWillReceiveProps.