Fix issues with controlled Select #1471
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NikoHelle
force-pushed
the
select-fixes-2025
branch
from
d1351c3 to
bb4d346
Compare
timwessman
force-pushed
the
select-fixes-2025
branch
from
bb4d346 to
c6aa9b2
Compare
timwessman
approved these changes
Feb 6, 2025
There was a problem hiding this comment.
Excellent work, Niko! This helps us a lot. Thanks for your contribution. Now we have a state that doesn't mix up. Yep, the main problem was onChange in the state.
I rebased this on the latest development, fixed the conflict and updated reference images (as the clearable property is now false as a default)
timwessman
force-pushed
the
select-fixes-2025
branch
from
c6aa9b2 to
ce24330
Compare
If onChange is in state, it keeps ref to the initial "optionGroups". Easily tested by selecting an option, changing language and selecting again. Lang is always the initial (fi)
Does not fix anything, but is a good practice.
onKeyDown is memoized, but the dependency array is empty, so the ref to the function does not update
The trigger function was not compared, so clicking an item second time, used old data in the memoized trigger.
When parent of the DataProvider is re-rendered and new data is passed, the old is cleared. onReset can restore data from the state the storages were before reset.
If props change, the "open" is reset to the value it was before re-render. Usually it was "false", so the menu will close once props change. This is not good behaviour when multiselect items are selected and list keeps closing. The "open" value is restored on each render.
… options are externally updated If groups are reset externally, the group label is not updated accordingly.
timwessman
force-pushed
the
select-fixes-2025
branch
from
ce24330 to
56a466e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixed following issues:
multiselect memoization issue: when selecting an option calls the
onChangewhich then refreshes props and re-renders the component, the selected option keeps has old data in the memoized "trigger" function. When an option with old "trigger" reference is unselected, the whole data set is reverted to old state and selections made after it are cleared.crash if keyboard is used after
onChangere-renders the component. Memoization issue in theuseKeyboard.onKeyDowndid not have correct dependencies."open" state was not kept consistently when
onChangecaused re-render. When component props change, theopencan be reverted to closed-state even if the component has internally opened it. This causes the multiselect close on each selection. The reset of the internal state is now handled by comparing old state with new props after a re-render.Multiselect groups with all options selected are not marked selected after re-render and clicking them had no effect.