-
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
Picker/PeoplePicker: Fix issue where default value wouldn't trigger suggestions #13480
Conversation
packages/office-ui-fabric-react/src/components/pickers/BasePicker.test.tsx
Outdated
Show resolved
Hide resolved
packages/office-ui-fabric-react/src/components/pickers/BasePicker.tsx
Outdated
Show resolved
Hide resolved
* This should be called when the user does something other than use text entry to trigger suggestions. | ||
* | ||
*/ | ||
private _userTriggeredSuggestions = () => { |
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.
👍 for moving this. Also a valuable refactor target in the future as there are tons of paths through these branches.
|
||
this.updateSuggestionsList(suggestions); | ||
|
||
this.setState({ |
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.
This seems to be new logic and not just a refactor. Intentional?
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.
Correct! This fixes an issue where a default value in the picker wouldn't have triggered a resolve suggestions.
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changes
Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B Baseline commit: 24e7759686ef88224d23ba35ac0e3add895357b6 (build) |
can you fill in a description of what the behavior was before/after? how do we test? |
packages/office-ui-fabric-react/src/components/pickers/BasePicker.test.tsx
Outdated
Show resolved
Hide resolved
packages/office-ui-fabric-react/src/components/pickers/BasePicker.test.tsx
Outdated
Show resolved
Hide resolved
packages/office-ui-fabric-react/src/components/pickers/BasePicker.test.tsx
Outdated
Show resolved
Hide resolved
packages/office-ui-fabric-react/src/components/pickers/BasePicker.test.tsx
Show resolved
Hide resolved
if (this.props.inputProps !== undefined && this.props.inputProps.onClick !== undefined) { | ||
this.props.inputProps.onClick(ev); | ||
} |
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.
if (this.props.inputProps !== undefined && this.props.inputProps.onClick !== undefined) { | |
this.props.inputProps.onClick(ev); | |
} | |
this.props.inputProps?.onClick?(ev); |
I think that would work :)
if (this.props.inputProps && this.props.inputProps.onFocus) { | ||
this.props.inputProps.onFocus(ev as React.FocusEvent<HTMLInputElement>); | ||
} |
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.
if (this.props.inputProps && this.props.inputProps.onFocus) { | |
this.props.inputProps.onFocus(ev as React.FocusEvent<HTMLInputElement>); | |
} | |
this.props.inputProps?.onFocus?(ev as React.FocusEvent<HTMLInputElement>); |
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.
Did we get these fixed? I think it was an issue with babel if I remember correctly.
…ker.test.tsx Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
🎉 Handy links: |
…uggestions (microsoft#13480) * Minor picker refactor, fix an issue where default value wouldn't trigger resolving value * Change files * update api * fix comments * remove unneeded test * fix minor issue * Update packages/office-ui-fabric-react/src/components/pickers/BasePicker.test.tsx Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com> Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Pull request checklist
$ yarn change
Description of changes
Original Behavior: If the picker's input contained some default value via
inputProps: {defaultVisibleValue: 'foo'}}
when the picker received focus, it would not show suggestions.The fix: The picker now attempts to callback to get suggestions if it currently contains a text value and does not have any current suggestions.
Focus areas to test
(optional)