-
Notifications
You must be signed in to change notification settings - Fork 327
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: Trigger multi combobox args after they are set, not before. #2343
Conversation
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.
Good job @mturoci! Everything is working as expected. Just one minor thing - using flushSync
can be risky and it is a last resort solution. In this case I think we could go without it (see the suggestion).
flushSync(() => { | ||
setSelected(keys => { | ||
const result = option.selected ? [...keys, String(option.key)] : keys.filter(key => key !== option.key) | ||
wave.args[m.name] = result | ||
return result | ||
}) | ||
}) | ||
|
||
if (m.trigger) wave.push() |
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.
Can using the flushSync
be avoided? This should be enough:
flushSync(() => { | |
setSelected(keys => { | |
const result = option.selected ? [...keys, String(option.key)] : keys.filter(key => key !== option.key) | |
wave.args[m.name] = result | |
return result | |
}) | |
}) | |
if (m.trigger) wave.push() | |
const result = option.selected ? [...selected, String(option.key)] : selected.filter(key => key !== option.key) | |
setSelected(result) | |
wave.args[m.name] = result | |
if (m.trigger) wave.push() |
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.
Our selected
state depends on its prev state thus a callback is necessary to make it safe.
flushSync is not risky, it just avoids batching state updates, but since we need to know the prev state, we do want sync updates anyway.
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.
Using your suggestion should work as well, but there are no guarantees from React that each user event like blur, tab, enter etc. commit state so I prefer to not depend on the internal implementation, especially when we are in the maintenance mode and I want each of us to spend as little time on this project as possible.
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.
Thanks for clarification! Your points make sense.
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.
LGTM!
Closes #2342