-
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 all 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 |
|---|---|---|
|
|
@@ -68,17 +68,17 @@ 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>(); | ||
| private _targetWindow: Window; | ||
| private _bounds: IRectangle; | ||
| private _maxHeight: number | undefined; | ||
| private _positionAttempts: number; | ||
| private _target: Element | MouseEvent | IPoint | null; | ||
| private _setHeightOffsetTimer: number; | ||
| private _hasListeners = false; | ||
| private _maxHeight: number | undefined; | ||
|
|
||
| constructor(props: ICalloutProps) { | ||
| super(props); | ||
|
|
@@ -136,6 +136,7 @@ export class CalloutContentBase extends BaseComponent<ICalloutProps, ICalloutSta | |
| positions: undefined | ||
| }); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| public componentDidMount() { | ||
|
|
@@ -172,8 +173,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 +185,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 = ( | ||
|
|
@@ -376,15 +376,22 @@ export class CalloutContentBase extends BaseComponent<ICalloutProps, ICalloutSta | |
| return this._bounds; | ||
| } | ||
|
|
||
| private _getMaxHeight(): number { | ||
| // Max height should remain as synchronous as possible, which is why it is not done using set state. | ||
| // It needs to be synchronous since it will impact the ultimate position of the callout. | ||
| private _getMaxHeight(): number | undefined { | ||
| if (!this._maxHeight) { | ||
| if (this.props.directionalHintFixed && this._target) { | ||
| const beakWidth = this.props.isBeakVisible ? this.props.beakWidth : 0; | ||
| const gapSpace = this.props.gapSpace ? this.props.gapSpace : 0; | ||
| // 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. |
||
| this.forceUpdate(); | ||
| } | ||
| }); | ||
| } 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(); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -154,7 +163,8 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
| // This slight delay is required so that we can unwind the stack, const react try to mess with focus, and then | ||
| // apply the correct focus. Without the setTimeout, we end up focusing the correct thing, and then React wants | ||
| // to reset the focus back to the thing it thinks should have been focused. | ||
| setTimeout(() => this._previousActiveElement!.focus(), 0); | ||
| // Note: Cannot be replaced by this._async.setTimout because those will be removed by the time this is called. | ||
| setTimeout(() => { this._previousActiveElement && this._previousActiveElement!.focus(); }, 0); | ||
| } | ||
|
|
||
| if (this.props.onMenuDismissed) { | ||
|
|
@@ -270,6 +280,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
| onScroll={ this._onScroll } | ||
| bounds={ bounds } | ||
| directionalHintFixed={ directionalHintFixed } | ||
| hidden={ this.props.hidden } | ||
| > | ||
| <div | ||
| role='menu' | ||
|
|
@@ -316,6 +327,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 && this._async.setTimeout(() => { this._previousActiveElement && this._previousActiveElement!.focus(); }, 0); | ||
| } | ||
|
|
||
| /** | ||
| * 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?