Skip to content

ContextualMenu: Update mousemove behavior #4589

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 6 commits into from
Apr 18, 2018
Merged

ContextualMenu: Update mousemove behavior #4589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
20bc7da
ContextualMenu: Update mousemove to only do anything if there is not …
jspurlin Apr 17, 2018
11bb565
rush change
jspurlin Apr 17, 2018
0b996e5
change spacing
jspurlin Apr 17, 2018
9f48835
Make sure we are clearing the enterTimerId after executing in the set…
jspurlin Apr 17, 2018
8bb49e3
fix missing line to set enterTimerId back to undefined if the item do…
jspurlin Apr 18, 2018
aeb4bd7
Merge branch 'master' of https://github.com/jspurlin/office-ui-fabric…
jspurlin Apr 18, 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 ...fice-ui-fabric-react/jspurlin-ContextualMenuUpdateMouseMoveBehavior_2018-04-17-17-55.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": "ContextualMenu: Update mousemove to only do anything if there is not another pending mouse enter/move/leave to improve the snappiness of the updates",
"type": "patch"
}
],
"packageName": "office-ui-fabric-react",
"email": "[email protected]"
}
6 changes: 5 additions & 1 deletion packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,9 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext

const targetElement = ev.currentTarget as HTMLElement;

if (!this._isScrollIdle || targetElement === this._targetWindow.document.activeElement as HTMLElement) {
if (!this._isScrollIdle ||
this._enterTimerId !== undefined ||
Copy link
Collaborator

@manishgarg1 manishgarg1 Apr 17, 2018

Choose a reason for hiding this comment

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

this._enterTimerId !== undefined [](start = 6, length = 32)

You actually do not need to do this. The following line of code will have the same affect and save you a few characters.

this._enterTimerId ||

Copy link
Contributor Author

@jspurlin jspurlin Apr 17, 2018

Choose a reason for hiding this comment

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

While it shouldn't be zero if it is a valid id, I couldn't find anything that technically says zero is not a valid return value. I don't want to make any assumptions which might make this code more brittle

Copy link
Collaborator

@manishgarg1 manishgarg1 Apr 17, 2018

Choose a reason for hiding this comment

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

OK sure. Your choice. I am certain it cannot be Zero. I know this because I have written such code more than once and never faced issues. That code has run for many years now.

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout
he returned timeoutID is a positive integer value which identifies the timer created by the call to setTimeout();

Copy link
Contributor Author

@jspurlin jspurlin Apr 17, 2018

Choose a reason for hiding this comment

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

According to IEEE 754-2008 zero can be signed and is positive by default (and can sometimes be negative), that being said it doesn't make sense to risk it

targetElement === this._targetWindow.document.activeElement as HTMLElement) {
return;
}

Expand Down Expand Up @@ -858,11 +860,13 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
const splitButtonContainer = this._splitButtonContainers.get(item.key);
this._onItemSubMenuExpand(item,
((item.split && splitButtonContainer) ? splitButtonContainer : targetElement) as HTMLElement);
this._enterTimerId = undefined;
}, this._navigationIdleDelay);
} else {
this._enterTimerId = this._async.setTimeout(() => {
this._onSubMenuDismiss(ev);
targetElement.focus();
this._enterTimerId = undefined;
}, timeoutDuration);
}
}
Expand Down