-
Notifications
You must be signed in to change notification settings - Fork 2.9k
ContextualMenu: Improve perf and allow buttons to benefit. #4524
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.
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
Changes from 5 commits
7dee606
52e01d8
7ec7fb7
d3d1402
0741d32
b27ea98
567ea80
f27ddad
e18c233
4d935c4
5a19690
520b744
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "changes": [ | ||
| { | ||
| "packageName": "office-ui-fabric-react", | ||
| "comment": "ContextualMenu/Button: Improve perf and add hiddenvariable to buttons", | ||
| "type": "minor" | ||
| } | ||
| ], | ||
| "packageName": "office-ui-fabric-react", | ||
| "email": "joschect@microsoft.com" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,6 +234,15 @@ export interface IButtonProps extends React.AllHTMLAttributes<HTMLAnchorElement | |
| * The default KeyCode is the down arrow. A value of null can be provided to disable the key codes for opening the button menu. | ||
| */ | ||
| menuTriggerKeyCode?: KeyCodes | null; | ||
|
|
||
| /** | ||
| * Menu will not be created or destroyed when opened or closed, instead it | ||
| * will be hidden. This will improve perf of the menu opening but could potentially | ||
| * impact overall perf by having more elemnts in the dom. Should only be used | ||
| * when perf is important. | ||
| * Note: This may increase the amount of time it takes for the button itself to mount. | ||
| */ | ||
| persistMenu?: boolean; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not quite sure what to name this and I am open to suggestions. |
||
| } | ||
|
|
||
| export enum ElementType { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,20 +130,29 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
| const newTarget = newProps.target; | ||
| this._setTargetWindowAndElement(newTarget!); | ||
| } | ||
| if (newProps.hidden !== this.props.hidden) { | ||
| if (newProps.hidden) { | ||
| this._onMenuClosed(); | ||
| } else { | ||
| this._onMenuOpened(); | ||
| this._previousActiveElement = this._targetWindow ? this._targetWindow.document.activeElement as HTMLElement : null; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Invoked once, both on the client and server, immediately before the initial rendering occurs. | ||
| public componentWillMount() { | ||
| const target = this.props.target; | ||
| this._setTargetWindowAndElement(target!); | ||
| this._previousActiveElement = this._targetWindow ? this._targetWindow.document.activeElement as HTMLElement : null; | ||
| if (!this.props.hidden) { | ||
| this._previousActiveElement = this._targetWindow ? this._targetWindow.document.activeElement as HTMLElement : null; | ||
| } | ||
| } | ||
|
|
||
| // Invoked once, only on the client (not on the server), immediately after the initial rendering occurs. | ||
| public componentDidMount() { | ||
| this._events.on(this._targetWindow, 'resize', this.dismiss); | ||
| if (this.props.onMenuOpened) { | ||
| this.props.onMenuOpened(this.props); | ||
| if (!this.props.hidden) { | ||
| this._onMenuOpened(); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -270,6 +279,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
| onScroll={ this._onScroll } | ||
| bounds={ bounds } | ||
| directionalHintFixed={ directionalHintFixed } | ||
| hidden={ this.props.hidden } | ||
| > | ||
| <div | ||
| role='menu' | ||
|
|
@@ -316,6 +326,16 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
| } | ||
| } | ||
|
|
||
| private _onMenuOpened() { | ||
| this._events.on(this._targetWindow, 'resize', this.dismiss); | ||
| this.props.onMenuOpened && this.props.onMenuOpened(this.props); | ||
| } | ||
|
|
||
| private _onMenuClosed() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need a similar call to this.props.onMenuDismissed here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not, onDismiss is a callback that would have triggered the hidden prop change farther up the tree. |
||
| this._events.off(this._targetWindow, 'resize', this.dismiss); | ||
| this._previousActiveElement && setTimeout(() => this._previousActiveElement!.focus(), 0); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we do a null check here on _previousActiveElement?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do, that's what
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the value can change when the callback in the set timeout gets invoked, cant it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point. I'll fix it. |
||
| } | ||
|
|
||
| /** | ||
| * Gets the focusZoneDirection by using the arrowDirection if specified, | ||
| * the direction specificed in the focusZoneProps, or defaults to FocusZoneDirection.vertical | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be expected to impact the time it takes to mount the button, perhaps that is worth calling out?