Skip to content
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.

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

#601 Modal bugs on react ^16 #645

Closed
wants to merge 4 commits into from
Closed

#601 Modal bugs on react ^16 #645

wants to merge 4 commits into from

Conversation

martindavid
Copy link

@martindavid martindavid commented Oct 20, 2017

  • Add ReactDOM.unstable_createPortal to support Fiber version.
  • Keep ReactDOM.unstable_renderSubtreeIntoContainer as a fallback for < 16 version.

Issue: #601

@TheSharpieOne
Copy link
Member

I thought I had commented on this earlier, but I apparently did not. The portal API is supposed to be used in the render AFAIK. This means that the logic would be similar, but separated. The render would return null for react <16 and would return the unstable_createPortal for react 16+. Similarly and separately, the opposite would happen in the renderIntoSubtree method; unstable_renderSubtreeIntoContainer would be called for react <15 and nothing would happen for react 16+.

@kinke
Copy link
Contributor

kinke commented Nov 28, 2017

Thanks @martindavid, I can confirm that this fixes the autoFocus issue for Modals with React 16 (e.g., escape key closing the modal etc.) and the fade-out animation as well. Oh btw, ReactDOM.createPortal() doesn't work with ReactDOM 16.1.1. Edit: Sorry, testing failure on my part, this does NOT work, just like ReactDOM.createPortal() isn't, at least not with React 16.1.1.

The portal API is supposed to be used in the render AFAIK.

@TheSharpieOne: The changes you propose would require more logic changes; renderIntoSubtree() is called in 3 places (show(), hide(), componentDidUpdate()).

@fantua
Copy link
Contributor

fantua commented Jan 5, 2018

@TheSharpieOne when this and #763 can be released? We have some troubles according to this issues.

Copy link
Collaborator

@virgofx virgofx left a comment

Choose a reason for hiding this comment

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

ReactDOM.createPortal required.

this._element
);
if (ReactDOM.unstable_createPortal) {
ReactDOM.unstable_createPortal(this.renderChildren(), this._element);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's no longer unstable_createPortal ... Should be ReactDOM.createPortal

https://reactjs.org/docs/portals.html

@geowarin
Copy link

I've been fiddling with the code for a couple of hours and I think @TheSharpieOne is correct, the createPortal call needs to happen in the render method, otherwise I cannot get it to work.

With a bit of effort, I made it work with React 16 and I can confirm that the focus problem is fixed.

Personnaly, I find it very difficult to make the two approaches (renderSubtreeIntoContainer vs createPortal) work together, it is a bit of work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants