-
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 3 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 |
|---|---|---|
|
|
@@ -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. | ||
| */ | ||
| 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 |
|---|---|---|
|
|
@@ -240,9 +240,10 @@ export interface ICalloutContentStyleProps { | |
| overflowYHidden?: boolean; | ||
|
|
||
| /** | ||
| * @deprecated will be removed in v6. Do not use. | ||
| * Max height applied to the content of a callout. | ||
| */ | ||
| contentMaxHeight?: number; | ||
| contentMaxHeight?: number | ||
|
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. shouldn't the semicolon still be here? |
||
|
|
||
| /** | ||
| * Background color for the beak and callout. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,7 +68,7 @@ export class CalloutContentBase extends BaseComponent<ICalloutProps, ICalloutSta | |
| directionalHint: DirectionalHint.bottomAutoEdge | ||
| }; | ||
|
|
||
| private _classNames: {[key in keyof ICalloutContentStyles]: string }; | ||
| private _classNames: { [key in keyof ICalloutContentStyles]: string }; | ||
| private _didSetInitialFocus: boolean; | ||
| private _hostElement = createRef<HTMLDivElement>(); | ||
| private _calloutElement = createRef<HTMLDivElement>(); | ||
|
|
@@ -172,8 +172,8 @@ export class CalloutContentBase extends BaseComponent<ICalloutProps, ICalloutSta | |
| target = this._getTarget(); | ||
| const { positions } = this.state; | ||
|
|
||
| const getContentMaxHeight: number = this._getMaxHeight() + this.state.heightOffset!; | ||
| const contentMaxHeight: number = calloutMaxHeight! && (calloutMaxHeight! < getContentMaxHeight) ? calloutMaxHeight! : getContentMaxHeight!; | ||
| const getContentMaxHeight: number | undefined = this._getMaxHeight() ? this._getMaxHeight() + this.state.heightOffset! : undefined; | ||
| const contentMaxHeight: number | undefined = calloutMaxHeight! && getContentMaxHeight && (calloutMaxHeight! < getContentMaxHeight) ? calloutMaxHeight! : getContentMaxHeight!; | ||
| const overflowYHidden = !!finalHeight; | ||
|
|
||
| const beakVisible = isBeakVisible && (!!target); | ||
|
|
@@ -184,15 +184,14 @@ export class CalloutContentBase extends BaseComponent<ICalloutProps, ICalloutSta | |
| className, | ||
| overflowYHidden: overflowYHidden, | ||
| calloutWidth, | ||
| contentMaxHeight, | ||
| positions, | ||
| beakWidth, | ||
| backgroundColor, | ||
| beakStyle | ||
| } | ||
| ); | ||
|
|
||
| const overflowStyle: React.CSSProperties = overflowYHidden ? { overflowY: 'hidden' } : {}; | ||
| const overflowStyle: React.CSSProperties = overflowYHidden ? { overflowY: 'hidden', maxHeight: contentMaxHeight } : { maxHeight: contentMaxHeight }; | ||
| const visibilityStyle: React.CSSProperties | undefined = this.props.hidden ? { visibility: 'hidden' } : undefined; | ||
| // React.CSSProperties does not understand IRawStyle, so the inline animations will need to be cast as any for now. | ||
| const content = ( | ||
|
|
@@ -384,7 +383,11 @@ export class CalloutContentBase extends BaseComponent<ICalloutProps, ICalloutSta | |
| // Since the callout cannot measure it's border size it must be taken into account here. Otherwise it will | ||
| // overlap with the target. | ||
| const totalGap = gapSpace + beakWidth! + BORDER_WIDTH * 2; | ||
| this._maxHeight = getMaxHeight(this._target, this.props.directionalHint!, totalGap, this._getBounds()); | ||
| this._async.requestAnimationFrame(() => { | ||
| if (this._target) { | ||
| this._maxHeight = getMaxHeight(this._target, this.props.directionalHint!, totalGap, this._getBounds()); | ||
|
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 to trigger a new render after this RAF call to actually set the max height?
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. Good point, I really should have had this been setstate.
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. So I ended up reverting this, to not use setstate. Ideally the maxHeight should be set as soon as possible, which can happen during the first available animation frame. |
||
| } | ||
| }); | ||
| } else { | ||
| this._maxHeight = this._getBounds().height! - BORDER_WIDTH * 2; | ||
| } | ||
|
|
||
| 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); | ||
| 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. Is _previousActiveElement guaranteed to not be null/undefined 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. It is not. I'll add a check |
||
| } | ||
|
|
||
| /** | ||
| * 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?