Update Portal #208

jquense · 2017-09-27T17:21:48Z

Our tests are SO FLAKEY.

@jochenberger sorry i was working off your PR but needed to see what was required with just the new API. I think we can still switch between LegacyPortal and Portal for back compat.

I don't love how this turned out I had to add a callback to Portal because it takes two render cycles to get something rendered in order to fire the focus() callback

jochenberger · 2017-09-28T06:34:31Z

Yeah, no worries, I just wanted to make some progress on the issue.

jochenberger · 2017-09-28T06:35:23Z

package.json

@@ -44,7 +44,7 @@
    "react-dom": "^0.14.9 || >=15.3.0"
  },
  "devDependencies": {
-    "@monastic.panic/component-playground": "^2.0.0",
+    "@monastic.panic/component-playground": "^3.2.2",


Wouldn't it be better to do that upgrade in a separate PR? Would be easier to review IMO.

I updated this to verify stuff was working apart from the tests which are flaky for me. I think it should be ok

jochenberger · 2017-09-28T06:37:28Z

test/ModalSpec.js


      render(
        <Modal show>
-          <div className='modal'>
+          <div className='modal'>q


using vi? ;-)

jochenberger · 2017-09-28T06:40:08Z

test/ModalSpec.js

@@ -11,6 +11,16 @@ import { render, shouldWarn } from './helpers';

 const $ = componentOrNode => jQuery(ReactDOM.findDOMNode(componentOrNode));

+
+class ErrorBoundary extends React.Component {


Is this related to the React.Children.only test failures (#207)? If so, a separate PR might be a good idea.

jochenberger · 2017-09-28T06:41:19Z

test/PositionSpec.js

@@ -20,7 +20,7 @@ describe('Position', function () {
    assert.equal(ReactDOM.findDOMNode(instance).nodeName, 'SPAN');
  });

-  it('Should warn about several children', function () {
+  xit('Should warn about several children', function () {


Shouldn't that be it again?

this is failing because of the other issues trying to be fixed in #207

add a FIXME

jochenberger · 2017-09-28T06:41:26Z

test/ModalSpec.js

-  it('Should throw with multiple children', function () {
-    expect(function(){
-      render(
+  xit('Should throw with multiple children', function () {


Shouldn't that be it again?

jochenberger · 2017-09-28T06:42:56Z

test/ModalSpec.js

@@ -353,8 +371,7 @@ describe('Modal', function () {
    });

    it('Should focus on the Modal when it is opened', function () {
-
-      document.activeElement.should.equal(focusableContainer);
+      expect(document.activeElement).to.equal(focusableContainer);


Does that make a difference?

it seems to locally. I'm not at all sure why

could be flakiness with the prototype manipulation stuff

jochenberger · 2017-09-28T06:43:43Z

test/ModalSpec.js

+    render(
+      <ErrorBoundary
+        onError={err => {
+          console.log('hiii')


You may want to remove this.

jochenberger · 2017-09-28T06:51:21Z

test/ModalSpec.js

@@ -11,6 +11,16 @@ import { render, shouldWarn } from './helpers';

 const $ = componentOrNode => jQuery(ReactDOM.findDOMNode(componentOrNode));

+
+class ErrorBoundary extends React.Component {
+  componentDidCatch(error, info) {


Do you want to upgrade the React dev-dependencies to 16? Otherwise you want want to add support for unstable_handleError.

taion

sorry for the delay

taion · 2017-10-04T05:15:48Z

src/Modal.js

@@ -472,7 +475,7 @@ class Modal extends React.Component {
    let autoFocus = this.props.autoFocus;
    let modalContent = this.getDialogElement();
    let current = activeElement(ownerDocument(this));
-    let focusInModal = current && contains(modalContent, current);
+    let focusInModal = modalContent && current && contains(modalContent, current);


see react-bootstrap/react-bootstrap#2726 (comment)

i think especially w/react 16 this might be a little flaky, since modalContent might not be the actual modal content... or at least not all of it

i'm not sure I understand that, don't we control this DOM node?

No – it's supposed to be the DOM node corresponding to children, but the actual node could be e.g. a comment or something per the linked React-Bootstrap issue if e.g. children don't end up rendering anything. Perhaps it could be something even weirder with React 16 – like a text node or something.

i'm not sure how to deal with this without supplying our own wrapping div, children should render a single element

we'd make a <RefHolder> component, attach a ref to it, then findDOMNode it... but this isn't really a regression or anything, so i guess this is fine for now

taion · 2017-10-04T05:16:43Z

src/Portal.js

 }

-export default Portal;
+export default ReactDOM.createPortal ? Portal : LegacyPortal;


taion · 2017-10-04T05:17:07Z

src/Portal.js

@@ -5,7 +5,7 @@ import ReactDOM from 'react-dom';

 import getContainer from './utils/getContainer';
 import ownerDocument from './utils/ownerDocument';
-
+import LegacyPortal from './LegacyPortal'


newline after this

taion · 2017-10-04T05:18:03Z

src/Portal.js

-      this._overlayInstance = null;
-    }
+  setContainer = (props = this.props) => {
+    this._portalContainerNode = getContainer(props.container, ownerDocument(this).body);


why not just use setState here?

can we handle setting this in the ctor in cases where it is defined there (for whatever reason?)

hmm, is there any meaningful impact to no longer creating a <div> here? what if the container does end up being document?

I don't think i understood what the div was doing in the old code, the container exists somewhere why were we then appending it to a different parent?

I'd like an earlier path but it takes some reworking. right now we do findDOMNode on the result of the function or the value, and we can't reasonable know when to not do that, or when it will error. I think we need to change the API to provide a DOM node specifically vs a component instance. e.g. () => findDOMNode(this), i think we do that for target in Position so it's probably a good idea anyway

@taion there are too many issues with doing a fast path. For instance, we default container to the owner document, which we get by doing findDOMNode on the component instance (Portal/Modal), in the early case that won't work because the component hasn't mounted yet.

these can can be individually handled, but gets harder across components, e.g Portal mounts early, and calls back to Modal's onShow which assumes/needs the owner document

Okay, forget the fast path then.

What are your thoughts around adding the <div>? Is that just a legacy thing to prevent trying to e.g. unmount the tree rooted at document?

I'm not sure honestly but it seems safe to remove. if someone wants to try and mount over their application that's sort of their problem :P my guess as to why it's there is so you don't blow away the dom that's already there when you render() but createPortal already appends into it's container

okay, so just to confirm, nothing will go wrong with using document as the portal node?

having the container be document wouldn't work before either. if it's document.body or any thing else i'll work fine

taion · 2017-10-04T05:20:46Z

src/LegacyPortal.js

+      this._mountOverlayTarget();
+      ReactDOM.unstable_renderSubtreeIntoContainer(
+        this, overlay, this._overlayTarget, (inst) => {
+          this._overlayInstance = inst


taion · 2017-10-04T05:20:49Z

src/LegacyPortal.js

+          this._overlayInstance = inst
+
+          if (this.props.onRendered) {
+            this.props.onRendered()


taion · 2017-10-04T05:23:20Z

test/PortalSpec.js

@@ -76,29 +70,8 @@ describe('Portal', function () {
    assert.equal(instance.overlay.getOverlayDOMNode(), null);
  });

-  it('Should render only an overlay', function() {


why does this test case no longer work?

i forget...

taion · 2017-10-04T05:23:46Z

examples/App.js

-        codeText={this.props.codeText}
-        collapsableCode
+        code={this.props.codeText}
+        collapsable


eww it's not "collapsible" like in react-bootstrap? ;p

jquense · 2017-10-10T14:10:21Z

lets get this in 👍

jharris4 · 2017-10-11T03:07:43Z

I tried this out (with React 16 and react-bootstrap next branch) in a project I have that's using modals, and it's looking good to me.

Let me know if there's anything I can do to help test this out

taion · 2017-10-11T04:40:57Z

src/Modal.js

@@ -472,7 +475,7 @@ class Modal extends React.Component {
    let autoFocus = this.props.autoFocus;
    let modalContent = this.getDialogElement();
    let current = activeElement(ownerDocument(this));
-    let focusInModal = current && contains(modalContent, current);
+    let focusInModal = modalContent && current && contains(modalContent, current);


we'd make a <RefHolder> component, attach a ref to it, then findDOMNode it... but this isn't really a regression or anything, so i guess this is fine for now

taion · 2017-10-11T04:41:50Z

test/PositionSpec.js

@@ -20,7 +20,7 @@ describe('Position', function () {
    assert.equal(ReactDOM.findDOMNode(instance).nodeName, 'SPAN');
  });

-  it('Should warn about several children', function () {
+  xit('Should warn about several children', function () {


add a FIXME

Update Portal

cda98af

jquense requested a review from taion September 27, 2017 17:24

jochenberger reviewed Sep 28, 2017

View reviewed changes

test/ModalSpec.js Outdated

render(

<Modal show>

<div className='modal'>

<div className='modal'>q

Copy link

Contributor

jochenberger Sep 28, 2017

Choose a reason for hiding this comment

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

using vi? ;-)

jochenberger reviewed Sep 28, 2017

View reviewed changes

test/ModalSpec.js Outdated

render(

<ErrorBoundary

onError={err => {

console.log('hiii')

Copy link

Contributor

jochenberger Sep 28, 2017

Choose a reason for hiding this comment

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

You may want to remove this.

jochenberger reviewed Sep 28, 2017

View reviewed changes

jochenberger mentioned this pull request Sep 28, 2017

WIP: Test failures with React 16 #207

Closed

jquense added 2 commits October 2, 2017 13:42

add legacy support

111d3f4

rm error stuff

7325e25

taion reviewed Oct 4, 2017

View reviewed changes

nits

387e865

taion approved these changes Oct 11, 2017

View reviewed changes

jharris4 mentioned this pull request Oct 11, 2017

Modal does not work with React 16 #188

Closed

nits

9e63f92

jquense merged commit dea8d71 into master Oct 14, 2017

jquense deleted the hot-fix-portal branch October 14, 2017 12:24

This was referenced Oct 16, 2017

switch to ReactDOM.createPortal #201

Closed

re-enable tests that were disabled for the Portal hotfix #216

Merged

		@@ -11,6 +11,16 @@ import { render, shouldWarn } from './helpers';

		const $ = componentOrNode => jQuery(ReactDOM.findDOMNode(componentOrNode));


		class ErrorBoundary extends React.Component {

Update Portal #208

Update Portal #208

Conversation

jquense commented Sep 27, 2017 • edited Loading

jochenberger commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquense commented Oct 10, 2017

jharris4 commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquense commented Sep 27, 2017 •

edited

Loading