Skip to content

ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes #4594

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.

Sign up for GitHub

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

Merged
jspurlin merged 12 commits into from
Apr 19, 2018
Merged

ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes #4594

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
46fdc33
added new paramater to callback
Apr 18, 2018
d853f6b
added change file
Apr 18, 2018
b3ae60d
updated the combobox type import
Apr 18, 2018
564fdad
changed callback to include all events instead of just keycode
Apr 18, 2018
d5f18be
made events more generic so user can parse it themselves
Apr 18, 2018
fc51d4b
made event optional
Apr 18, 2018
baaf37c
Merge https://github.com/OfficeDev/office-ui-fabric-react into chiech…
Apr 18, 2018
d6d02e9
re-arranged function call and added more any events
Apr 18, 2018
836b00a
removed unneeded parameter added
Apr 18, 2018
b3551c7
updated change file
Apr 19, 2018
f322efa
fixed type
Apr 19, 2018
55b0465
changed event to any to prevent crashes
Apr 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions common/changes/office-ui-fabric-react/chiechan-newComboboxCallback_2018-04-18-00-34.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "ComboBox: Added KeyCode pressed as additional paramater to onChanged callback",

@jspurlin jspurlin Apr 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the title to the PR) need to be updated

"type": "minor"
}
],
"packageName": "office-ui-fabric-react",
"email": "chiechan@microsoft.com"
}
18 changes: 9 additions & 9 deletions packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
* @param index - the index to set (or the index to set from if a search direction is provided)
* @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) {

@jspurlin jspurlin Apr 18, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a better name for the new parameter, submittedInput isn't very clear

@jspurlin jspurlin Apr 18, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@chang47 chang47 Apr 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

const { onChanged, onPendingValueChanged } = this.props;
const { currentOptions } = this.state;
let { selectedIndices } = this.state;
Expand Down Expand Up @@ -798,7 +798,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {

// Did the creator give us an onChanged callback?
if (onChanged) {
onChanged(option, index);
onChanged(option, index, undefined, submittedInput);
}

// if we have a new selected index,
Expand Down Expand Up @@ -882,7 +882,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
/**
* Submit a pending value if there is one
*/
private _submitPendingValue() {
private _submitPendingValue(submittedInput?: KeyCodes) {
const {
onChanged,
allowFreeform,
Expand Down Expand Up @@ -914,14 +914,14 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
currentPendingValue.length + (this._comboBox.value.selectionEnd - this._comboBox.value.selectionStart) === pendingOptionText.length) ||
(this._comboBox.value && this._comboBox.value.inputElement && this._comboBox.value.inputElement.value.toLocaleLowerCase() === pendingOptionText)
)) {
this._setSelectedIndex(currentPendingValueValidIndex);
this._setSelectedIndex(currentPendingValueValidIndex, SearchDirection.none, submittedInput);
this._clearPendingInfo();
return;
}
}

if (onChanged) {
onChanged(undefined, undefined, currentPendingValue);
onChanged(undefined, undefined, currentPendingValue, submittedInput);
} else {
// If we are not controlled, create a new option
const newOption: IComboBoxOption = { key: currentPendingValue, text: currentPendingValue };
Expand All @@ -940,10 +940,10 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
} else if (currentPendingValueValidIndex >= 0) {
// Since we are not allowing freeform, we must have a matching
// to be able to update state
this._setSelectedIndex(currentPendingValueValidIndex);
this._setSelectedIndex(currentPendingValueValidIndex, SearchDirection.none, submittedInput);
} else if (currentPendingValueValidIndexOnHover >= 0) {
// If all else failed and we were hovering over an item, select it
this._setSelectedIndex(currentPendingValueValidIndexOnHover);
this._setSelectedIndex(currentPendingValueValidIndexOnHover, SearchDirection.none, submittedInput);
}

// Finally, clear the pending info
Expand Down Expand Up @@ -1471,7 +1471,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {

switch (ev.which) {
case KeyCodes.enter:
this._submitPendingValue();
this._submitPendingValue(ev.which);
if (this.props.multiSelect && isOpen) {
this.setState({
currentPendingValueValidIndex: index
Expand Down Expand Up @@ -1499,7 +1499,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
case KeyCodes.tab:
// On enter submit the pending value
if (!this.props.multiSelect) {
this._submitPendingValue();
this._submitPendingValue(ev.which);
}

// If we are not allowing freeform
Expand Down
4 changes: 2 additions & 2 deletions packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { ISelectableOption } from '../../utilities/selectableOption/SelectableOp
import { ISelectableDroppableTextProps } from '../../utilities/selectableOption/SelectableDroppableText.types';
import { IStyle, ITheme } from '../../Styling';
import { IButtonStyles } from '../../Button';
import { IRenderFunction } from '../../Utilities';
import { IRenderFunction, KeyCodes } from '../../Utilities';

@jspurlin jspurlin Apr 18, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You no longer need KeyCodes

import { IComboBoxClassNames } from './ComboBox.classNames';

export interface IComboBox {
Expand Down Expand Up @@ -53,7 +53,7 @@ export interface IComboBoxProps extends ISelectableDroppableTextProps<IComboBox>
* 2) a manually edited value is submitted. In this case there may not be a matched option if allowFreeform is also true
* (and hence only value would be true, the other parameter would be null in this case)
*/
onChanged?: (option?: IComboBoxOption, index?: number, value?: string) => void;
onChanged?: (option?: IComboBoxOption, index?: number, value?: string, submittedInput?: KeyCodes) => void;

/**
* Callback issued when the user changes the pending value in ComboBox
Expand Down