ContextualMenu: Improve perf and allow buttons to benefit.#4524
ContextualMenu: Improve perf and allow buttons to benefit.#4524joschect merged 12 commits intomicrosoft:masterfrom
Conversation
joschect
requested review from
christiango,
dzearing,
manishgarg1,
micahgodbolt and
montselozanod
| * impact overall perf by having more elemnts in the dom. Should only be used | ||
| * when perf is important. | ||
| */ | ||
| persistMenu?: boolean; |
There was a problem hiding this comment.
I'm not quite sure what to name this and I am open to suggestions.
|
|
||
| private _onMenuClosed() { | ||
| this._events.off(this._targetWindow, 'resize', this.dismiss); | ||
| setTimeout(() => this._previousActiveElement!.focus(), 0); |
There was a problem hiding this comment.
Is _previousActiveElement guaranteed to not be null/undefined here?
There was a problem hiding this comment.
It is not. I'll add a check
| this.props.onMenuOpened && this.props.onMenuOpened(this.props); | ||
| } | ||
|
|
||
| private _onMenuClosed() { |
There was a problem hiding this comment.
Do we need a similar call to this.props.onMenuDismissed here?
There was a problem hiding this comment.
We do not, onDismiss is a callback that would have triggered the hidden prop change farther up the tree.
| * Max height applied to the content of a callout. | ||
| */ | ||
| contentMaxHeight?: number; | ||
| contentMaxHeight?: number |
There was a problem hiding this comment.
shouldn't the semicolon still be here?
| 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()); |
There was a problem hiding this comment.
Do we need to trigger a new render after this RAF call to actually set the max height?
There was a problem hiding this comment.
Good point, I really should have had this been setstate.
There was a problem hiding this comment.
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.
| /** | ||
| * 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 |
There was a problem hiding this comment.
This would be expected to impact the time it takes to mount the button, perhaps that is worth calling out?
|
Thanks for taking a look at this issue so quickly! |
…hect/office-ui-fabric-react into improve-contextualmenu-perf
|
|
||
| private _onMenuClosed() { | ||
| this._events.off(this._targetWindow, 'resize', this.dismiss); | ||
| this._previousActiveElement && setTimeout(() => this._previousActiveElement!.focus(), 0); |
There was a problem hiding this comment.
Should we do a null check here on _previousActiveElement?
There was a problem hiding this comment.
We do, that's what this._previousActiveElement && does.
There was a problem hiding this comment.
But the value can change when the callback in the set timeout gets invoked, cant it?
There was a problem hiding this comment.
That's a good point. I'll fix it.
3 participants
Pull request checklist
$ npm run changeDescription of changes
Improve perf by removing a non-async sizing check and made max-height inline to reduce reflows. Also added the ability to have contextualmenus render hidden like callouts and button menus to render hidden as well.
Focus areas to test
(optional)