Skip to content

Adding onPendingValueChanged callback #4105

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 17 commits into from
Mar 7, 2018
Merged

Adding onPendingValueChanged callback #4105

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ebe922e
Adding onPendingValueSet callback
madosal Feb 27, 2018
d1c5014
Adding rush change file
madosal Feb 27, 2018
51f3aa8
Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…
madosal Feb 27, 2018
c9626cd
Creating two props for preview and revert preview
madosal Feb 28, 2018
edb9901
Fixing some spaces
madosal Feb 28, 2018
22bf253
Changed description
manueldosal Feb 28, 2018
6c72b47
Adding currentPendingValue back to address free form
madosal Mar 1, 2018
d38795d
Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…
madosal Mar 1, 2018
df4f4ef
Merge branch 'ComboBox_Add_onPendingValueSet_Callback' of https://git…
madosal Mar 1, 2018
b03155c
Going back to onPendingValueChanged
madosal Mar 2, 2018
ff7d2e6
Adding some comments
madosal Mar 2, 2018
215d6f1
Renaming variable to sendUndefinedPendingValue
madosal Mar 3, 2018
33f7e26
Merge with master
madosal Mar 5, 2018
1acb059
Reducing the logic to only send one callback
madosal Mar 6, 2018
46d2fd4
Simplifying logic
madosal Mar 6, 2018
b58225b
Changing the comment description
manueldosal Mar 7, 2018
d98eb23
Fixing newPendingIndex condition
madosal Mar 7, 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 ...nges/office-ui-fabric-react/ComboBox_Add_onPendingValueSet_Callback_2018-02-27-01-11.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 adding onPendingValueSet callback prop to run when pending value is changed",
"type": "patch"
}
],
"packageName": "office-ui-fabric-react",
"email": "madosal@microsoft.com"
}
57 changes: 55 additions & 2 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 @@ -125,6 +125,8 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {

private _isScrollIdle: boolean;

private _isInPreview: boolean;

@jspurlin jspurlin Mar 1, 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.

What is _isInPreview? Is it true when there's a pending value?

@manueldosal manueldosal Mar 1, 2018

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.

This is when the pending values is not undefined


private readonly _scrollIdleDelay: number = 250 /* ms */;

private _scrollIdleTimeoutId: number | undefined;
Expand Down Expand Up @@ -197,6 +199,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
const {
isOpen,
focused,
currentOptions,

@manueldosal manueldosal Feb 28, 2018

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.

Removing this that I don't need it anymore

selectedIndex,
currentPendingValueValidIndex
} = this.state;
Expand Down Expand Up @@ -236,6 +239,8 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
this._select();
}

this._notifyPreviewChanged(prevState);

if (isOpen && !prevState.isOpen && onMenuOpen) {
onMenuOpen();
}
Expand Down Expand Up @@ -347,7 +352,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
styles={ this._getCaretButtonStyles() }
role='presentation'
aria-hidden={ isButtonAriaHidden }
data-is-focusable={false}
data-is-focusable={ false }
tabIndex={ -1 }
onClick={ this._onComboBoxClick }
iconProps={ buttonIconProps }
Expand Down Expand Up @@ -692,7 +697,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
* @param searchDirection - the direction to search along the options from the given index
*/
private _setSelectedIndex(index: number, searchDirection: SearchDirection = SearchDirection.none) {
const { onChanged } = this.props;
const { onChanged, onRevertPreviewExecute } = this.props;
const { selectedIndex, currentOptions } = this.state;

// Find the next selectable index, if searchDirection is none
Expand All @@ -713,6 +718,12 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
selectedIndex: index
});

// If ComboBox value is changed, revert preview first
if (this._isInPreview && onRevertPreviewExecute) {
onRevertPreviewExecute();
this._isInPreview = false;
}

// Did the creator give us an onChanged callback?
if (onChanged) {
onChanged(option, index);
Expand Down Expand Up @@ -1241,6 +1252,48 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
}
}

private _notifyPreviewChanged(prevState: IComboBoxState) {
const { onPreviewExecute, onRevertPreviewExecute } = this.props;
const {
currentOptions,
currentPendingValueValidIndex,
currentPendingValueValidIndexOnHover
} = this.state;

if (onPreviewExecute && onRevertPreviewExecute) {

@jspurlin jspurlin Mar 1, 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 pulling this if check to the top of the function negating it and returning if it's true so the whole function isn't nested inside of the check

let sendRevert = false;
let currentPendingIndex = -1;

if (currentPendingValueValidIndexOnHover !== prevState.currentPendingValueValidIndexOnHover) {

if (this._indexWithinBounds(currentOptions, currentPendingValueValidIndexOnHover)) {
currentPendingIndex = currentPendingValueValidIndexOnHover;
} else {
sendRevert = true;
}
}

if (currentPendingIndex < 0 && currentPendingValueValidIndex !== prevState.currentPendingValueValidIndex) {

if (this._indexWithinBounds(currentOptions, currentPendingValueValidIndex)) {
currentPendingIndex = currentPendingValueValidIndex;
} else {
sendRevert = true;
}
}

if (this._isInPreview && (sendRevert || currentPendingIndex >= 0)) {
onRevertPreviewExecute();
this._isInPreview = false;
}

if (!this._isInPreview && currentPendingIndex >= 0) {
onPreviewExecute(currentOptions[currentPendingIndex], currentPendingIndex);
this._isInPreview = true;
}
}

@jspurlin jspurlin Feb 28, 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.

What about the case when a user enters a value that doesn't already exist as an option on the comboBox (when allowFreeform is true)?

@manueldosal manueldosal Feb 28, 2018

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.

That case I'm not sure if it's handled in Excel, so I'm skipping it. In legacy ribbon only available menu items have live preview.

@jspurlin jspurlin Mar 1, 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.

Can you add comments for all of these if checks? This seems overly complex and it could potentially be restructured to be more simple and straight forward.

}

/**
* Sets the isOpen state and updates focusInputAfterClose
*/
Expand Down
11 changes: 11 additions & 0 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 @@ -48,6 +48,17 @@ export interface IComboBoxProps extends ISelectableDroppableTextProps<IComboBox>
*/
onChanged?: (option?: IComboBoxOption, index?: number, value?: string) => void;

/**
* Callback issued when the user changes the pending value index in ComboBox
* This includes hovering or keyboarding to a menu item
*/
onPreviewExecute?: (option?: IComboBoxOption, index?: number) => void;

/**
* Callback issued when user leaves the menu item in the ComboBox menu
*/
onRevertPreviewExecute?: () => void;

@jspurlin jspurlin Mar 1, 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.

Do we actually need a revert callback? There's either going to be a new pending value or it's there's no pending value. I think onPendingValueChanged could handle both cases


/**
* Function that gets invoked when the ComboBox menu is launched
*/
Expand Down
32 changes: 17 additions & 15 deletions packages/office-ui-fabric-react/src/components/ComboBox/examples/ComboBox.Basic.Example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,20 @@ export class ComboBoxBasicExample extends React.Component<{}, {
value?: string;
}> {
private _testOptions =
[{ key: 'Header', text: 'Theme Fonts', itemType: SelectableOptionMenuItemType.Header },
{ key: 'A', text: 'Arial Black' },
{ key: 'B', text: 'Times New Roman' },
{ key: 'C', text: 'Comic Sans MS' },
{ key: 'divider_2', text: '-', itemType: SelectableOptionMenuItemType.Divider },
{ key: 'Header1', text: 'Other Options', itemType: SelectableOptionMenuItemType.Header },
{ key: 'D', text: 'Option d' },
{ key: 'E', text: 'Option e' },
{ key: 'F', text: 'Option f' },
{ key: 'G', text: 'Option g' },
{ key: 'H', text: 'Option h' },
{ key: 'I', text: 'Option i' },
{ key: 'J', text: 'Option j', disabled: true },
];
[{ key: 'Header', text: 'Theme Fonts', itemType: SelectableOptionMenuItemType.Header },
{ key: 'A', text: 'Arial Black' },
{ key: 'B', text: 'Times New Roman' },
{ key: 'C', text: 'Comic Sans MS' },
{ key: 'divider_2', text: '-', itemType: SelectableOptionMenuItemType.Divider },
{ key: 'Header1', text: 'Other Options', itemType: SelectableOptionMenuItemType.Header },
{ key: 'D', text: 'Option d' },
{ key: 'E', text: 'Option e' },
{ key: 'F', text: 'Option f' },
{ key: 'G', text: 'Option g' },
{ key: 'H', text: 'Option h' },
{ key: 'I', text: 'Option i' },
{ key: 'J', text: 'Option j', disabled: true },
];

private _fontMapping: { [key: string]: string } = {
['Arial Black']: '"Arial Black", "Arial Black_MSFontService", sans-serif',
Expand Down Expand Up @@ -81,7 +81,9 @@ export class ComboBoxBasicExample extends React.Component<{}, {
onFocus={ () => console.log('onFocus called') }
onBlur={ () => console.log('onBlur called') }
onMenuOpen={ () => console.log('ComboBox menu opened') }
// tslint:enable:jsx-no-lambda
onPreviewExecute={ (option, pendingIndex) => console.log('Preview value was changed. Pending index: ' + pendingIndex) }
onRevertPreviewExecute={ () => console.log('Preview value was reverted') }
// tslint:enable:jsx-no-lambda
/>

<PrimaryButton
Expand Down