Update Portal Example: Render Modal children when Modal is inserted in the DOM tree

[darrenscerri]

Fixes #272

Not sure if this is clear enough. Maybe it might be better to move the Modal implementation to a separate section along with a beefier explanation?

Also noticed that there is a Modal implementation in a Codepen linked from the Portals page (https://codepen.io/gaearon/pen/yzMaBd) that may cause the mentioned issues.

[reactjs-bot]

Deploy preview ready!

Built with commit f3fdc9b

https://deploy-preview-275--reactjs.netlify.com

[bvaughn]

There's no point to call createPortal with a null value. Maybe it would be nicer to do something like this:

class Modal extends React.Component {
  _container = document.createElement('div');

  // Keep track of when the modal element is added the DOM
  state = {
    mounted: false
  };

  componentDidMount() {
    modalRoot.appendChild(this._container);

    this.setState({
      mounted: true
    });
  }

  componentWillUnmount() {
    modalRoot.removeChild(this._container);
  }

  render() {
    // Don't render the portal until the component has mounted,
    // So the portal can safely access the DOM.
    if (!this.state.mounted) {
      return null;
    }

    return ReactDOM.createPortal(
      this.props.children,
      this._container,
    );
  }
}

Of course, now that I think about this, this implementation also has the potential to cause problems. For example, imagine if someone used this Modal component like so:

// `Child` in this example is a class component
class Parent extends React.Component {
  _childRef = null;

  componentDidMount() {
    // Someone might expect _childRef to be set
  }

  render() {
    return (
      <Modal>
        <Child ref={this._setRef} />
      </Modal>
    );
  }

  _setRef = ref => {
    this._childRef = ref;
  }
}

[darrenscerri]

The ref issue is analogous to the Child's componentDidMount issue in the sense that it still doesn't respect certain assumptions, so I think the example in the PR would be a no-go. I cannot think of a 100% correct solution that would mitigate these issues without introducing side-effects in the constructor, componentWillMount or render methods of Modal.

Honestly, the current Modal implementation wouldn't cause any issues in the majority of use cases, unless it's used in a way that its children need to perform any DOM actions that assume a mounted DOM (measuring dimensions, checking ancestors, etc.), or having autoFocus in any of its descendants. Still, this breaks the unwritten (and justified) assumption that a componentDidMount is invoked when the component is in fact mounted and available in the DOM tree. This also may introduce inconsistencies in components as they may behave differently if mounted in a Modal or not.

Maybe the best way forward is to add a note in the docs highlighting these possible concerns?

[bvaughn]

Maybe the best way forward is to add a note in the docs highlighting these possible concerns?

Agreed! Given the trade-offs we've discussed, maybe the best option at the moment (unless someone else can think of a better one?) is just to add a note inline to the existing example, as you say.

[bvaughn]

(Would you like to submit a new PR that adds that comment? Or update this one to do that instead?)

[darrenscerri]

I can update this one and then squash commits when merging if that's ok.

[bvaughn]

Sounds great!

[bvaughn]

Cool. Thanks for the discussion on this issue 😄

[darrenscerri]

Thanks a lot 😄 👍