-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[Pickers] Add async debounce option to BasePicker #4163
[Pickers] Add async debounce option to BasePicker #4163
Conversation
@antonlabunets Debounce for PeoplePicker component needed for Yammer webpart in Sharepoint, please review? |
/** | ||
* A callback for when the input has been changed | ||
*/ | ||
onInputChanged?: (filter: string) => void; |
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.
I'm very hesitant to add another onInputChange, there is already one that is used which is passed down to the autofill. Is there anyway that could be used instead?
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.
Yeah, I can remove this, not critical to our needs, just what I thought would be a nice-to-have...
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.
Removed, thanks!
3f0a09a
to
e67cf62
Compare
@joschect Thanks for the review, ready for a second pass. I'm happy to polish up anything else that might need work... |
* e.g. If a second input change happens within the resolveDelay time, the timer will start over. | ||
* Only until after the timer completes will onResolveSuggestions be called. | ||
*/ | ||
resolveDelay?: number; |
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.
Is there a default? Should it default to something? Will it get treated as no delay without it?
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 question, right now no default is set so that if the resolveDelay
prop isn't passed in, there's no delay but still passes through debounced function.
I felt an opt-in was safer than setting default, but if you feel this should be a default behavior, we can add one. The delay I'm using in the examples is 300 ms
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.
Approved with one more question/suggestion
* master: Applying package updates. Polyfill react create ref (microsoft#4182) [Pickers] Add async debounce option to BasePicker (microsoft#4163) Facepile conditionally rendering aria-describedby (microsoft#4183) Applying package updates. Addressing Issue microsoft#4174 - 5.59.0 crashing in <ImageBase> (microsoft#4185) Website fix: Remove PureComponent (microsoft#4186) Add Keytip and KeytipLayer to experiments export (microsoft#4184)
Pull request checklist
$ npm run change
Description of changes
Add mechanism to debounce searching in pickers
Add resolveDelay to allow consumer to add a delay on calling resolveSuggestions (to not burden services and not waste cycles on throw away searches)
Focus areas to test
PeoplePicker