[ContextualMenu] Fixes useTargetWidth property#3943
[ContextualMenu] Fixes useTargetWidth property#3943martellaj merged 5 commits intomicrosoft:masterfrom
Conversation
martellaj
requested review from
MLoughry,
christiango,
dzearing,
joschect and
micahgodbolt
|
@joschect Hey, Jon - you're required on this one. It's a pretty small change - can you please take a look when you get a chance? This affects one of the main UX flows in OWA. Thanks! |
| @@ -247,7 +247,9 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |||
| let contextMenuStyle; | |||
| let targetAsHtmlElement = this._target as HTMLElement; | |||
| if ((useTargetWidth || useTargetAsMinWidth) && targetAsHtmlElement && targetAsHtmlElement.offsetWidth) { | |||
There was a problem hiding this comment.
you're checking offsetWidth, but calling getBoundingClientRect, while it's probably fine we should still change it to be consistent.
There was a problem hiding this comment.
What's the appropriate check here then? I would've thought checking offsetWidth initially was unnecessary anyway.
| let targetAsHtmlElement = this._target as HTMLElement; | ||
| if ((useTargetWidth || useTargetAsMinWidth) && targetAsHtmlElement && targetAsHtmlElement.offsetWidth) { | ||
| let targetWidth = targetAsHtmlElement.offsetWidth; | ||
| const targetBoundingRect = targetAsHtmlElement.getBoundingClientRect(); |
There was a problem hiding this comment.
This should never have been in the render function in the first place. Calling offsetWidth/getBoundingClientRect (and other stuff) can trigger a reflow which is bad for performance. Ideally we'll store this and not call it again, and when we do call it it should be async. Move all of this to a separate function that get's called asynchronously.
There was a problem hiding this comment.
Any guidance on this? There doesn't seem to be any async/await patterns that exist in OUFR.
There was a problem hiding this comment.
I'm not sure how to include an async function in the render function. Should the calculation be done in life-cycle method?
There was a problem hiding this comment.
Take a look at _updateAsyncPosition() in callout. It uses requestAnimationFrame which essentially bundles the measures/renders separately, so instead of there being dom changes then measuring then dom changes again and then re-measuring (and so on), it tells the browser to complete dom changes, then measures after reflows are complete.
Really contextualmenu shouldn't be measuring anything. But yes, it should be part of the lifecycle.
| } | ||
| } | ||
|
|
||
| private _setTargetWidthAsync() { |
There was a problem hiding this comment.
@joschect Here's the attempt at asynchronously getting the target's width. Let me know what you think.
| const targetBoundingRect = targetAsHtmlElement.getBoundingClientRect(); | ||
|
|
||
| this.setState({ | ||
| targetWidth: targetBoundingRect.width - 2 /* Need to account for border width */ |
There was a problem hiding this comment.
targetWidth [](start = 8, length = 11)
Since this async, how do we make sure this value is set when the render API is called?
There was a problem hiding this comment.
It gets called in componentWillMount, so the value should be available in the render function. We'll have this issue either way since @joschect said even reading offsetWidth should be moved to an async function. Can someone more familiar with this pattern in OUFR help solve the problem? Do you have any suggestions, @manishgarg1?
There was a problem hiding this comment.
Or if you have a better way to solve it, feel free to take it over. The issue has been open since November with no activity and we need a fix. Thanks!
There was a problem hiding this comment.
It's okay if the value is not set at first render, although it may cause apparent resizing. You would need to provide a default value though.
|
@manishgarg1 @joschect Can I revert back to my original change where I measure to get the appropriate width? It's a separate issue (that already exists today) that it's done in the |
|
It's probably okay to revert to your original fix for now. |
This reverts commit 442c9c0.
|
@manishgarg1 The async bit is a separate issue, and should be addressed by someone more familiar with the code. I've reverted back to my original change to measure width appropriately. @joschect Your sign-off is still required since you're the owner. Please let me know if there's any more required changes. Thanks! |
martellaj
dismissed
manishgarg1’s stale review
These comments pertain to elements that are no longer pertinent to this PR.
* master: (71 commits) Applying package updates. Delete initials_2018-02-07-13-49.json Delete initials_2018-02-07-13-49.json Delete jolore-addingWorkWeekDateRange_2018-01-24-01-39.json Delete initials_2018-02-07-13-49.json Delete jolore-addingWorkWeekDateRange_2018-01-24-01-39.json Delete initials_2018-02-07-13-49.json Update .npmrc Cleaning Up Console Log in CommandBar Test (microsoft#4011) Convert Overlay to mergeStyles (microsoft#3978) ScrollablePane SCSS to MergeStyles Part 1: File Structure (microsoft#4008) [ContextualMenu] Fixes useTargetWidth property (microsoft#3943) DetailsList: Consider groups when setting aria-rowcount List: Add a _notifyPageChanges function (microsoft#3990) Applying package updates. Migrating Coachmark to main Package, Added a beak component and updated experiment PositioningContainer. (microsoft#3919) Update package.json Added enum for triggering menu with arrow keys and bool to allow it or not (microsoft#3950) BaseExtendedPicker: Hook up onPaste (microsoft#3885) FocusUtil: fix getPreviousElement to include previous sibling elements correctly (microsoft#3928) ...
4 participants
Pull request checklist
$ npm run changeDescription of changes
Instead of using
offsetWidth, which rounds the actual width value to the nearest integer, I changed the behavior to get the target element's bounding rect and use that element's actualwidthproperty. So instead of rendering aContextualMenuthat is slightly wider than the target, it'll now render with the proper width.