Move <Layer> to use React portals when available by cschleiden · Pull Request #4724 · microsoft/fluentui

cschleiden · 2018-05-01T20:56:52Z

Updated.

This has a new dependency @dzearing. I need "enzyme-to-json" for the snapshots.

dmarcey · 2018-05-02T16:57:27Z

It looks like there is some code in CalloutContent.base.tsx that is relying on virtual parentage for determining whether or not it should dismiss callouts when focus is lost.

protected _dismissOnLostFocus(ev: Event) {
const target = ev.target as HTMLElement;
const clickedOutsideCallout = this._hostElement.current && !elementContains(this._hostElement.current, target);

if (
  (!this._target && clickedOutsideCallout) ||
  ev.target !== this._targetWindow &&
  clickedOutsideCallout &&
  ((this._target as MouseEvent).stopPropagation ||
    (!this._target || (target !== this._target && !elementContains(this._target as HTMLElement, target))))) {
  this.dismiss(ev);
}

}

With these changes, we're no longer setting the virtual parent of the layer, so this is causing the a ContextualMenu to dismiss itself as soon a sub menu is opened. I think we'll either need to do something where we wrap the call to ReactDOM.createPortal in a with a ref, so that we can set the virtual parent of the layer to be an element that is a DOM child as well.

Something like this:

return <span ref={ this._rootElement }>
  { ReactDOM.createPortal(
    (
      <Fabric className={ classNames.content }>
        { this.props.children }
      </Fabric>
    ),
    layerElement
  ) }
</span>

and then in componentDidMount():

if (this._rootElement && this._rootElement.current && this._layerElement) {
  setVirtualParent(this._layerElement, this._rootElement.current);
}

cschleiden · 2018-05-03T16:06:10Z

-   * Used for notifying applicable Layers that a host is available/unavailable and to re-evaluate Layers that
-   * care about the specific host.
-   */
-  public static notifyHostChanged(id: string) {


thinking about this change, I'll move the methods back and just redirect to the new ones to not break compat

betrue-final-final · 2018-05-03T18:45:06Z

Ok, which one is right?

joschect · 2018-05-03T18:59:21Z

@betrue-final-final Definitely previously accepted one. "Left center" means to the left of the target.

cschleiden · 2018-05-03T21:09:14Z

Yep, looking into the screener tests

cschleiden · 2018-05-05T00:32:50Z

Interesting... fixed the callout, broke the contextual menu. The actual change is not that complicated, but other components seem to indirectly depend on the order of operations in the <Layer> component.

joschect · 2018-05-05T01:13:03Z

The odd thing about the contextualmenu break is now it doesn't appear at all. I'll be there's an error in the contextualmenu somewhere.

joschect · 2018-05-05T01:13:35Z

Also they really shouldn't be this dependant on order. They should only care about when they get mounted. I'll pull this branch over the weekend and see if I can't debug.

cschleiden · 2018-05-05T01:23:32Z

@joschect I've fixed the Callout, it shouldn't try to position when the target is not yet available. That looks better now. Context menu should also be fixed, a recent change I made didn't set the virtualparent for initial render, which breaks the context menu sub menus.

K, fixed. If no target is given, I needed to position anyway.

joschect · 2018-05-05T23:01:41Z

@cschleiden Looks great. Thanks for fixing it! This changes are pretty awesome and should simplify a bunch of code.

@dzearing We might want to make sure that people know that this change is coming since there could be some subtle side effects.

cschleiden · 2018-05-06T17:16:06Z

@joschect @dzearing I've sent a headsup mail to the v-team alias last week

joschect

I did some spot checking in pickers, dropdown, and combobox to make sure the new portal didn't impact some of the event handling. Looks good!

cschleiden · 2018-05-06T23:56:41Z

Merged master and now there are problems with combo and dropdown SSR tests, will fix.

cschleiden · 2018-05-07T14:43:46Z

Had to pin the markdown-to-jsx version due to this change https://github.com/probablyup/markdown-to-jsx/pull/174/files

cschleiden · 2018-05-07T15:41:30Z

@micahgodbolt @ThomasMichon can you take a look?

cliffkoh · 2018-05-07T23:51:54Z

+import { PortalLayerBase } from './PortalLayer.base';
 import { getStyles } from './Layer.styles';

+const portalSupport: boolean = !!createPortal;


Very interesting... Just wondering out loud - would this cause problems for a consumer of OUFR 5.x using the ES2015 module imports?

So, I think (and I might be wrong here), if a consumer is on React 15 and consumes the ES2015 exports (/lib-es2015/), this would be an invalid import due to the static nature of ES2015 modules... This would then break all usages of Fabric, for instance, if it was in the browser.

* Use Portals when available for Callout * Update shrinkwrap * Set virtual parent for initial render * Only wait for target when required * Update shrinkwrap * update * Prevent markdown-to-jsx from upgrading * Add patch file for example app base change * Update Layer.tsx

)" This reverts commit 0283f2a.

* Revert "Move <Layer> to use React portals when available (#4724)" This reverts commit 0283f2a. * Add change file * Address comment * Re-add enzyme-to-json * Pin markdown-to-jsx dependency

* master: (52 commits) Marqueeselection style update (microsoft#4803) Applying package updates. FocusZone: Add the ability to stop focus from propagating outside the FocusZone (microsoft#4823) Unknown persona coin (microsoft#4809) Applying package updates. BaseButton: Allow Alt + Down on menu buttons to open the menu (microsoft#4807) Applying package updates. Website: Scroll to top of page on navigation (microsoft#4810) Applying package updates. ActivityItem: fix getstyles (microsoft#4802) Mark Slider#ValuePosition enum as deprecated as it is unused. (microsoft#4799) Icon: ensure `styles` still works. (microsoft#4805) Popup: Added check for onBlur to prevent focus setting focus to be incorrectly disabled when closing a menu in Chrome (microsoft#4804) Update package.json Move <Layer> to use React portals when available (microsoft#4724) Fix breadcrumb rendering issue when overflow item is at last index (microsoft#4787) Fixes focus issue for contextual menus (microsoft#4744) Remove redundant selected items prop on BaseExtendedPicker (microsoft#4769) SearchBox: New prop for turning off icon animation (microsoft#4794) Add functions to ContextualMenuItem to open and close menus on command (microsoft#4741) ...

* Move <Layer> to use React portals when available (#4724) * Use Portals when available for Callout * Update shrinkwrap * Set virtual parent for initial render * Only wait for target when required * Update shrinkwrap * update * Prevent markdown-to-jsx from upgrading * Add patch file for example app base change * Update Layer.tsx * Add event blocking to maintain traditional behavior that can be disabled by new prop. * Remove old LayerBase implementation. * Add portal utility helper and use in Tooltip. * Revert incorrect typing. * Change files. * Update users-cschleid-portalLayers_2018-05-03-15-52.json * Update snapshots for merge. * Add utility unit tests. * List dependency info for CI builds. * Fix test break due to change in enzyme-adapter-react-16. * Add Layer event boundary unit tests. * Allow Tooltip to appear within portal when target is in same portal. * PR feedback. * Clarifying comments.

cschleiden requested a review from ThomasMichon as a code owner May 1, 2018 20:56

cschleiden force-pushed the users/cschleid/portalLayers branch from 802fa18 to fd2866e Compare May 2, 2018 18:09

cschleiden changed the title ~~[EARLY DRAFT] Initial portal version~~ Move <Layer> to use React portals when available May 2, 2018

cschleiden requested review from joschect and micahgodbolt as code owners May 2, 2018 23:48

cschleiden commented May 3, 2018

View reviewed changes

cschleiden force-pushed the users/cschleid/portalLayers branch from 2c55fb0 to 448ffd5 Compare May 4, 2018 22:45

cschleiden and others added 2 commits May 4, 2018 17:03

Use Portals when available for Callout

bc390a7

Update shrinkwrap

515ef2f

cschleiden force-pushed the users/cschleid/portalLayers branch from 47ca75e to 515ef2f Compare May 5, 2018 00:16

Set virtual parent for initial render

26b8bce

Only wait for target when required

af52125

cschleiden added 2 commits May 6, 2018 10:21

merge from master

a61a73b

Update shrinkwrap

17db094

joschect approved these changes May 6, 2018

View reviewed changes

update

6c85f33

cschleiden added 2 commits May 6, 2018 18:13

Prevent markdown-to-jsx from upgrading

2574dbd

Add patch file for example app base change

e84b584

Merge branch 'master' into users/cschleid/portalLayers

c57669c

cliffkoh reviewed May 7, 2018

View reviewed changes

Update Layer.tsx

10ee98a

dzearing merged commit 0283f2a into microsoft:master May 8, 2018

MLoughry added a commit to MLoughry/office-ui-fabric-react that referenced this pull request May 10, 2018

Revert "Move <Layer> to use React portals when available (microsoft#4724

5beb8c5

)" This reverts commit 0283f2a.

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Uh oh!

Conversation

cschleiden commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmarcey commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cschleiden May 3, 2018

Choose a reason for hiding this comment

Uh oh!

betrue-final-final commented May 3, 2018

Uh oh!

joschect commented May 3, 2018

Uh oh!

cschleiden commented May 3, 2018

Uh oh!

cschleiden commented May 5, 2018

Uh oh!

joschect commented May 5, 2018

Uh oh!

joschect commented May 5, 2018

Uh oh!

cschleiden commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joschect commented May 5, 2018

Uh oh!

cschleiden commented May 6, 2018

Uh oh!

joschect left a comment

Choose a reason for hiding this comment

Uh oh!

cschleiden commented May 6, 2018

Uh oh!

cschleiden commented May 7, 2018

Uh oh!

cschleiden commented May 7, 2018

Uh oh!

cliffkoh May 7, 2018

Choose a reason for hiding this comment

Uh oh!

cliffkoh May 8, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cschleiden commented May 1, 2018 •

edited

Loading

dmarcey commented May 2, 2018 •

edited

Loading

cschleiden commented May 5, 2018 •

edited

Loading