Adding onPendingValueChanged callback by manueldosal · Pull Request #4105 · microsoft/fluentui

manueldosal · 2018-02-27T01:14:32Z

Pull request checklist

[] Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

ComboBox: adding onPendingValueSet callback prop to run when pending value is changed.

Focus areas to test

Tested that hovering and using arrows through menu items runs the callback.

Checked with these test cases:

Hover over menu items
Arrowing over menu items
Move mouse out of the menu.
Hover over a menu item and start arrowing down.
Arrow down to a menu item, then hover over a different menu item.
Hover over a menu item M, arrow down to a menu item N, move mouse over M.
Change the text in the input.
Autocomplete on input.
Click on a menu item.
Enter in input when changed value.

jspurlin · 2018-02-27T15:16:51Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

+      } else if (currentPendingValueValidIndexOnHover !== prevState.currentPendingValueValidIndexOnHover) {
+        onPendingValueSet(currentOptions[currentPendingValueValidIndexOnHover], currentPendingValueValidIndexOnHover, undefined);
+      } else if (currentPendingValueValidIndex !== prevState.currentPendingValueValidIndex) {
+        onPendingValueSet(currentOptions[currentPendingValueValidIndex], currentPendingValueValidIndex, undefined);


Consider pulling this into a function to keep componentDidUpdate from getting too messy

The else if statements are practically the same, consider simplifying to set a local variable for the index, and if that exists then call the callback using that index

jspurlin · 2018-02-27T15:18:04Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.types.ts

+   * Callback issued when the user changes the pending value in ComboBox
+   * If now value is set, we send undefined parameters
+   */
+  onPendingValueSet?: (option?: IComboBoxOption, index?: number, value?: string) => void;


Maybe onPendingValueChanged is better name

…c-react into ComboBox_Add_onPendingValueSet_Callback

manueldosal · 2018-02-28T22:59:17Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

    const {
      isOpen,
      focused,
+      currentOptions,


Removing this that I don't need it anymore

manueldosal · 2018-02-28T23:07:23Z

Checked with these test cases:

Hover over menu items
Arrowing over menu items
Move mouse out of the menu.
Hover over a menu item and start arrowing down.
Arrow down to a menu item, then hover over a different menu item.
Hover over a menu item M, arrow down to a menu item N, move mouse over M.
Change the text in the input.
Autocomplete on input.
Click on a menu item.
Enter in input when changed value.

jspurlin · 2018-02-28T23:26:30Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

+        onPreviewExecute(currentOptions[currentPendingIndex], currentPendingIndex);
+        this._isInPreview = true;
+      }
+    }


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)?

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.

…c-react into ComboBox_Add_onPendingValueSet_Callback

…hub.com/manueldosal/office-ui-fabric-react into ComboBox_Add_onPendingValueSet_Callback

jspurlin · 2018-03-01T01:50:46Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

+      currentPendingValueValidIndexOnHover
+    } = this.state;
+
+    if (onPreviewExecute && onRevertPreviewExecute) {


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

jspurlin · 2018-03-01T01:54:54Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.types.ts

+   * 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, value?: string) => void;


This name isn't very clear, I think onPendingValueChanged is more clear

jspurlin · 2018-03-01T02:00:09Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.types.ts

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


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

jspurlin · 2018-03-01T02:10:56Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx


  private _isScrollIdle: boolean;

+  private _isInPreview: boolean;


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

This is when the pending values is not undefined

jspurlin · 2018-03-01T02:15:55Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

+        onPreviewExecute(currentPendingIndex ? currentOptions[currentPendingIndex] : undefined, currentPendingIndex, pendingValue);
+        this._isInPreview = true;
+      }
+    }


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.

jspurlin

Consider having one callback, it should simplify things

jspurlin · 2018-03-05T18:53:56Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

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


Consider changing this to true by default and then setting it to false inside of the if statements. It would remove the need for an else statement

jspurlin · 2018-03-05T18:57:24Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

+
+    // Notify the new pending value if any of currentPendingIndex or pendingValue was changed.
+    if (!this._hasPendingValue && (currentPendingIndex || pendingValue)) {
+      onPendingValueChanged(currentPendingIndex ? currentOptions[currentPendingIndex] : undefined, currentPendingIndex, pendingValue);


Does currentPendingIndex ? currentOptions[currentPendingIndex] : undefined work as expected if currentPendingIndex === 0?

nataaad · 2018-04-20T13:01:49Z

Hi,
I don't understand this comment :

Notify when there is a new pending index/value. Also, if there is a pending value, it needs to send undefined.

Why send undefined instead of value is not undefined ?

madosal added 2 commits February 26, 2018 17:09

Adding onPendingValueSet callback

ebe922e

Adding rush change file

d1c5014

jspurlin reviewed Feb 27, 2018

View reviewed changes

madosal added 2 commits February 27, 2018 10:27

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

51f3aa8

…c-react into ComboBox_Add_onPendingValueSet_Callback

Creating two props for preview and revert preview

c9626cd

manueldosal commented Feb 28, 2018

View reviewed changes

Fixing some spaces

edb9901

manueldosal changed the title ~~Adding onPendingValueSet callback~~ Adding onPreviewExecute and onRevertPreviewExecute callback Feb 28, 2018

manueldosal changed the title ~~Adding onPreviewExecute and onRevertPreviewExecute callback~~ Adding onPreviewExecute and onRevertPreviewExecute callbacks Feb 28, 2018

Changed description

22bf253

jspurlin reviewed Feb 28, 2018

View reviewed changes

madosal added 3 commits February 28, 2018 17:24

Adding currentPendingValue back to address free form

6c72b47

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

d38795d

…c-react into ComboBox_Add_onPendingValueSet_Callback

Merge branch 'ComboBox_Add_onPendingValueSet_Callback' of https://git…

df4f4ef

…hub.com/manueldosal/office-ui-fabric-react into ComboBox_Add_onPendingValueSet_Callback

jspurlin reviewed Mar 1, 2018

View reviewed changes

jspurlin requested changes Mar 1, 2018

View reviewed changes

madosal added 3 commits March 2, 2018 15:09

Going back to onPendingValueChanged

b03155c

Adding some comments

ff7d2e6

Renaming variable to sendUndefinedPendingValue

215d6f1

jspurlin reviewed Mar 5, 2018

View reviewed changes

madosal added 3 commits March 5, 2018 13:07

Merge with master

33f7e26

Reducing the logic to only send one callback

1acb059

Simplifying logic

46d2fd4

manueldosal changed the title ~~Adding onPreviewExecute and onRevertPreviewExecute callbacks~~ Adding onPendingValueChanged callback Mar 7, 2018

manueldosal and others added 2 commits March 7, 2018 10:52

Changing the comment description

b58225b

Fixing newPendingIndex condition

d98eb23

jspurlin approved these changes Mar 7, 2018

View reviewed changes

jspurlin merged commit 48ad19d into microsoft:master Mar 7, 2018

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019


		private _isScrollIdle: boolean;

		private _isInPreview: boolean;

Conversation

manueldosal commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manueldosal commented Feb 28, 2018

Uh oh!

jspurlin Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspurlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nataaad commented Apr 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

manueldosal commented Feb 27, 2018 •

edited

Loading

jspurlin Feb 28, 2018 •

edited

Loading