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

values props are not working with multi option #13

Closed
jayasurian123 opened this issue Jun 27, 2019 · 6 comments
Closed

values props are not working with multi option #13

jayasurian123 opened this issue Jun 27, 2019 · 6 comments

Comments

@jayasurian123
Copy link

I was trying to set the values prop externally to the Select dropdown. It would work perfectly when the reference of the values props are same. But if we are using redux or any other external state management it will not work.

I created a small reproducible scenario here.
https://codesandbox.io/s/external-clear-68i29

I have also had a look at the code and the issue which i suspect is

if (prevProps.values !== this.props.values && prevProps.values === prevState.values) {

We may need to have a proper array comparison here.
eg:

 JSON.stringify(prevProps.values) ===  JSON.stringify(prevState.values)

Let me know if you need any more info from my side!

thanks for the nice lib.

@sanusart
Copy link
Owner

Hey @jayasurian123 please checkout slightly modified fork of your sandbox (note the line 58-59 for correction)
https://codesandbox.io/s/external-clear-w0ukz Does this addresses the issue?

@jayasurian123
Copy link
Author

hi @sanusart I know that would work 😇.

The example was just a simplification of showing external state management like redux state would not work with the current implementation. Thats why cloned the value state.

I have come across this because of this scenario.
Selecting Dropdown options shows the widgets / modules in the page. And once widget/ modules are closed the dropdown should reflex the changes. So need to have an external state management to store the state.

It would be always better to compare the array instead of their reference. Or removing prevProps.values === prevState.values (not sure that would break some other feature).

@sanusart
Copy link
Owner

@jayasurian123 got it.
Will check this out with cloned. That comparation needed for proper internal and external change detection. I guess you right and real equality comparation needed.
Thanks for clarifying this.

@jayasurian123
Copy link
Author

@sanusart 👍

@sanusart
Copy link
Owner

@jayasurian123 Seems like scenario you posted works with 3.1.0. Thanks for taking time to report and investigate!

@jayasurian123
Copy link
Author

@sanusart Thanks a ton. It's a fantastic lib. Keep up the great work 👍

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

2 participants