ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes#4594
Conversation
| * @param searchDirection - the direction to search along the options from the given index | ||
| */ | ||
| private _setSelectedIndex(index: number, searchDirection: SearchDirection = SearchDirection.none) { | ||
| private _setSelectedIndex(index: number, searchDirection: SearchDirection = SearchDirection.none, submittedInput?: KeyCodes) { |
There was a problem hiding this comment.
Consider using a better name for the new parameter, submittedInput isn't very clear
There was a problem hiding this comment.
Also, should this be a keyCode or the whole event? Can't a value be submitted by changing the value in the input and clicking away? What happens currently in that case?
There was a problem hiding this comment.
@jspurlin Yes a value can be submitted by changing the value. I wanted to abstract as much information as necessary from the user so I used keycodes, but you do bring a valid point, maybe just passing the whole event will be better for the user to make an more informed decision. What do you think?
For our specific case, we currently only check for tabs so focus would go to wherever we clicked on.
| * Submit a pending value if there is one | ||
| */ | ||
| private _submitPendingValue(submittedInput?: KeyCodes) { | ||
| private _submitPendingValue(submitPendingValueEvent: React.KeyboardEvent<HTMLElement | Autofill> | React.FocusEvent<HTMLInputElement>) { |
There was a problem hiding this comment.
Consider wrapping this to multiple lines
There was a problem hiding this comment.
Traditionally events that are passed to callbacks in fabric use type any to get around having to specify all of this
There was a problem hiding this comment.
Thank goodness, I was debating about type safety vs having a complex parameter, and chose type safety. But looking around now, you're right, there are instances where we just use any.
| private _onItemClick(index: number | undefined): () => void { | ||
| return (): void => { | ||
| this._setSelectedIndex(index as number); | ||
| private _onItemClick(index: number | undefined): (ev: React.MouseEvent<any> | React.FormEvent<any>) => void { |
There was a problem hiding this comment.
same comment here, this could just be type any
| * (and hence only value would be true, the other parameter would be null in this case) | ||
| */ | ||
| onChanged?: (option?: IComboBoxOption, index?: number, value?: string, submittedInput?: KeyCodes) => void; | ||
| onChanged?: (option?: IComboBoxOption, index?: number, value?: string, submitPendingValueEvent?: React.KeyboardEvent<any> | React.FocusEvent<any> | React.MouseEvent<any> | React.FormEvent<any>) => void; |
| import { IStyle, ITheme } from '../../Styling'; | ||
| import { IButtonStyles } from '../../Button'; | ||
| import { IRenderFunction } from '../../Utilities'; | ||
| import { IRenderFunction, KeyCodes } from '../../Utilities'; |
There was a problem hiding this comment.
You no longer need KeyCodes
| * @param searchDirection - the direction to search along the options from the given index | ||
| */ | ||
| private _setSelectedIndex(index: number, searchDirection: SearchDirection = SearchDirection.none) { | ||
| private _setSelectedIndex(index: number, searchDirection: SearchDirection = SearchDirection.none, submitPendingValueEvent?: any) { |
There was a problem hiding this comment.
Did you look at making this new parameter the second parameter? Doing that might make it to where you have to pass searchDirection in the cases where you don't need
| this._setSelectedIndex(index as number); | ||
| private _onItemClick(index: number | undefined): (ev: any) => void { | ||
| return (ev: React.MouseEvent<any> | React.FormEvent<any>): void => { | ||
| this._setSelectedIndex(index as number, undefined, ev); |
There was a problem hiding this comment.
Should this be searchDirection.none instead of undefined?
| return (): void => { | ||
| this._setSelectedIndex(index as number); | ||
| private _onItemClick(index: number | undefined): (ev: any) => void { | ||
| return (ev: React.MouseEvent<any> | React.FormEvent<any>): void => { |
There was a problem hiding this comment.
this can be updated to any
| "changes": [ | ||
| { | ||
| "packageName": "office-ui-fabric-react", | ||
| "comment": "ComboBox: Added KeyCode pressed as additional paramater to onChanged callback", |
There was a problem hiding this comment.
This (and the title to the PR) need to be updated
chang47
changed the title
* master: Applying package updates. ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes (microsoft#4594) Remove usage of Number.NaN (microsoft#4615) Update createRef to match the new React 16.3 api (microsoft#4598) Update Breadcrumb.base.tsx Fix minor typos (microsoft#4607) Remove unused variables and enable no-unused-variable (microsoft#4608) Add optional overflowIndex prop to Breadcrumb (microsoft#4609) Applying package updates. Reenable bundlesize in yaml (microsoft#4590) Fix more index imports (microsoft#4604)
* master: DetailsRow: Flexshrink fix (microsoft#4622) [TextField] Implemented masking (microsoft#3783) Applying package updates. ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes (microsoft#4594) Remove usage of Number.NaN (microsoft#4615)
2 participants
Problem:
When submitting a value in a combo box, depending on what input is used to submit the value, from an accessibility standpoint, we want to take different actions that would accurately reflect the input that was used whenever we submit our value for callbacks.
For example:
Pressing enter, we would submit the current value in the combo box, however pressing tab would also submit the current value, but it would also move focus to the next element.
Currently, we have no way of differentiating what inputs were used on the component so we can't deal with what happens with a combo box correctly after its input has been changed.
Solution:
We exposed the keycode that was pressed (if any) to the callback so that the user can do whatever they deem necessary to create an accessible experienece using a combo box
Validation:
Seeing as the only change done was adding an extra parameter to the callback, nothing should be broken, however I have played with with all the combo boxes submitting my values by hitting: enter, tab, escape, and click and ensured that the submitted code gets called, there's no errors, and that the behavior remained the same as before.