Skip to content

[ContextualMenu] Fixes useTargetWidth property #3943

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.

Sign up for GitHub

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

Merged
martellaj merged 5 commits into from
Feb 16, 2018
Merged

[ContextualMenu] Fixes useTargetWidth property #3943

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
50528e0
Fixes useTargetWidth usage for ContextualMenu
martellaj Feb 11, 2018
1e6a98c
Adds change file
martellaj Feb 11, 2018
442c9c0
Gets targetWidth asynchronously
martellaj Feb 12, 2018
f3cdb18
Revert "Gets targetWidth asynchronously"
martellaj Feb 14, 2018
d667782
Accounts for border width
martellaj Feb 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions common/changes/office-ui-fabric-react/joem-fix-callout-targetwidth_2018-02-11-00-32.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "Fixes useTargetWidth prop for ContextualMenu",
"type": "patch"
}
],
"packageName": "office-ui-fabric-react",
"email": "joem@microsoft.com"
}
6 changes: 4 additions & 2 deletions packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

@joschect joschect Feb 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're checking offsetWidth, but calling getBoundingClientRect, while it's probably fine we should still change it to be consistent.

@martellaj martellaj Feb 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the appropriate check here then? I would've thought checking offsetWidth initially was unnecessary anyway.

let targetWidth = targetAsHtmlElement.offsetWidth;
const targetBoundingRect = targetAsHtmlElement.getBoundingClientRect();

@joschect joschect Feb 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@martellaj martellaj Feb 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any guidance on this? There doesn't seem to be any async/await patterns that exist in OUFR.

@martellaj martellaj Feb 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to include an async function in the render function. Should the calculation be done in life-cycle method?

@joschect joschect Feb 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const targetWidth = targetBoundingRect.width;

if (useTargetWidth) {
contextMenuStyle = {
width: targetWidth
Expand All @@ -271,7 +273,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
}
return (
<Callout
{...calloutProps}
{ ...calloutProps }
target={ useTargetPoint ? targetPoint : target }
isBeakVisible={ isBeakVisible }
beakWidth={ beakWidth }
Expand Down