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 #6245 Select searchValue not changeable when value and searchValue are controlled #6272

Merged
merged 10 commits into from
Jun 13, 2024

Conversation

floriankapaun
Copy link
Contributor

@floriankapaun floriankapaun commented May 23, 2024

Fixes #6270
Fixes #6245

@rtivital
Copy link
Member

It breaks dynamic data functionality. You can see that in "Data changes over time" story. Video of behavior differences:

2024-05-23.17.54.43.mov
2024-05-23.17.55.58.mov

@floriankapaun
Copy link
Contributor Author

Damn. Missed that because the tests were all looking good. Thanks for spotting this @rtivital.

I will look into this again but I feel like the original behavior of that story was not appropriate either.

In my opinion the selected option should get reset (to null/undefined) whenever the underlying data changes and the currently selected option is not part of the new dataset anymore. Setting the selected option to something else that came with the new dataset does not really make sense to me but I might be missing something so please correct me if you have different thoughts on that.

@floriankapaun
Copy link
Contributor Author

Nevermind. After looking at the story again I noticed that the value stays the same and it's only the label that is changing. I guess then it's fine the way it is.

@floriankapaun floriankapaun changed the title Fix #6270 Select searchValue not changeable when value and searchValue are controlled Fix #6245 Select searchValue not changeable when value and searchValue are controlled May 24, 2024
@floriankapaun
Copy link
Contributor Author

Please check again @rtivital

@rtivital
Copy link
Member

This is not a good solution, this makes @mantine/hooks depend on fast-deep-equal which is not acceptable. Also, there is no plans to adduseDeepMemo hook to the @mantine/hooks package.

@floriankapaun
Copy link
Contributor Author

Can understand your decision @rtivital. Do you have a better idea to fix this bug?

@floriankapaun
Copy link
Contributor Author

Would you approve adding the logic/idea from useMemoWithDeepComparison to the component itself without depending on the fast-deep-equal library? Or do we need to figure out something entirely different @rtivital?

@rtivital
Copy link
Member

No, it is a flawed logic even without the dependency. The comparison for deep equality will be performed at each render because data is not usually memoized, it will have a significant impact on performance for the large data sets.

@floriankapaun
Copy link
Contributor Author

I again took some time to work on this and tried to figure out a better approach. Hope you will like this one now @rtivital. 🙂

@rtivital rtivital merged commit fe87119 into mantinedev:master Jun 13, 2024
1 check passed
@rtivital
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants