-
Notifications
You must be signed in to change notification settings - Fork 75
[LG-5469] fix(combobox): initialize value immediately #3480
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 2b0d1fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Pull request overview
This PR fixes a flickering issue in the Combobox component by initializing the selection state immediately during render rather than after the first render through a useEffect hook. The flickering occurred because styles dependent on isMultiselectWithSelections were being calculated based on null selection in the initial render.
Changes:
- Replaced deferred initialization of selection state via useEffect with immediate initialization using useState initializer function
- Moved validation logic inline to the useState initializer
68f1a50 to
090f3c2
Compare
| const [selection, setSelection] = useState<SelectValueType<M> | null>(() => { | ||
| if (initialValue) { | ||
| if (isArray(initialValue)) { | ||
| // Ensure the values we set are real options | ||
| const filteredValue = | ||
| initialValue.filter(value => isValueValid(value)) ?? []; | ||
| return filteredValue as SelectValueType<M>; | ||
| } else { | ||
| if (isValueValid(initialValue as string)) { | ||
| return initialValue; | ||
| } | ||
| } | ||
| } |
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.
It appears we have some regression in our tests where initial values are not being properly set on single selection. I believe this might be because later on in the file we have a useEffect that checks if the selection has changed, and updates the selection.
// onSelect
// Side effects to run when the selection changes
useEffect(() => {
const hasSelectionChanged =
!isUndefined(prevSelection) &&
((isArray(selection) && !isNull(prevSelection)) ||
isString(selection) ||
isNull(selection)) &&
!isEqual(selection, prevSelection);
if (hasSelectionChanged) {
onSelect();
}
}, [onSelect, prevSelection, selection]);Because we previously had the extra render, selection was initially null then the selection would be set in initialValue, triggering the 2nd render where hasSelectionChanged was now true. Given that it's now being set synchronously, that's no longer true and is never being set. Perhaps the inputValue state needs to also be initialized synchronously as well when initialValue is provided for single-select.
✍️ Proposed changes
There is a flickering related to re-rendering the Combobox (with multiselect) - see codesandbox.
This is issue is visible in Compass - https://jira.mongodb.org/browse/COMPASS-9891
Looking into the implementation, I found that the selection has been initialized after the 1st render (in the 1st render it is always
null). Several styles depend onisMultiselectWithSelections, so this has been causing the flickering (there is still some flicker but not as obvious). Moving the initialization fromuseEffecttosetStateavoids this.🎟 Jira ticket: LG-5469
✅ Checklist
For new components
For bug fixes, new features & breaking changes
pnpm changesetand documented my changes🧪 How to test changes