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

Child ref null in componentDidUpdate of parent containing a Modal component #601

Closed
michaeldzjap opened this Issue Sep 29, 2017 · 14 comments

Comments

Projects
7 participants
@michaeldzjap

michaeldzjap commented Sep 29, 2017

Issue description

  • version #5.0.0-alpha.2
  • components: Modal, ModalBody, ModalHeader

After updating to React 16 I noticed that a child component of ModalBody containing a ref is null in the componentDidUpdate() lifecycle method. Not sure if this is caused by React v16 or if it is due to reactstrap. It seems wrong to me, but perhaps there is a good reason for this that I am overlooking? When you close the modal again you do see that the ref is defined.

Steps to reproduce issue

See this CodeSandbox. Open the JS console and click the button to open the modal. The console prints null (tested in latest Chrome).

@michaeldzjap michaeldzjap changed the title from Child ref null in componentDidUpdate of Modal to Child ref null in componentDidUpdate of parent containing a Modal component Sep 29, 2017

@TheSharpieOne

This comment has been minimized.

Member

TheSharpieOne commented Sep 29, 2017

This is related to facebook/react#9808
It looks like we will need to switch to unstable_createPortal but provide a fallback to unstable_renderSubtreeIntoContainer when unstable_createPortal doesn't exist to keep supporting 15.x.

@TheSharpieOne TheSharpieOne added this to the v5 milestone Sep 29, 2017

@Chima1707

This comment has been minimized.

Chima1707 commented Sep 30, 2017

@TheSharpieOne sure that was the reason. If you dont mind, can i work on this for this hacktoberfest?

@virgofx

This comment has been minimized.

Collaborator

virgofx commented Sep 30, 2017

@TheSharpieOne Even better ... ReactDOM.createPortal(child, container) which marked as stable. Just need to change requirement to >= R16. Maybe we can do it next version?

@TheSharpieOne

This comment has been minimized.

Member

TheSharpieOne commented Sep 30, 2017

Sure, go for it. I would also look at how material UI handled it
Also, yeah, I saw the stable name and that it was aliased to the unstable name. For better compatibility I would use the unstable name (support the R16 betas)

@Chima1707

This comment has been minimized.

Chima1707 commented Sep 30, 2017

Ok, guys will send a fix likely this weekend.

@TheSharpieOne

This comment has been minimized.

Member

TheSharpieOne commented Oct 2, 2017

@Chima1707 how is this coming along?

@Chima1707

This comment has been minimized.

Chima1707 commented Oct 2, 2017

@TheSharpieOne am investigating the problem, will give my update by the end of tomorrow

@leetercola

This comment has been minimized.

leetercola commented Oct 5, 2017

Curious about status, this is currently breaking unit tests in jest & enzyme with react 16.

@TheSharpieOne

This comment has been minimized.

Member

TheSharpieOne commented Oct 7, 2017

As am I. @Chima1707 any update? If you are not able to complete it, it's not a big deal, just let us know.

@Chima1707

This comment has been minimized.

Chima1707 commented Oct 7, 2017

I have not been able to complete it, please can someone take a look at it?

@martindavid

This comment has been minimized.

martindavid commented Oct 17, 2017

@TheSharpieOne is anyone working on this at the moment? I'd like to give it a try.

@TheSharpieOne

This comment has been minimized.

Member

TheSharpieOne commented Oct 17, 2017

@martindavid go for it.

@supergibbs supergibbs added this to To Do in v5.0.0-beta via automation Jan 29, 2018

@fantua

This comment has been minimized.

Contributor

fantua commented Jan 29, 2018

Looks like it's still doesn't resolved.

According to this comment I can try to:

  1. Use react-portal (uses React v16 and its official API for creating portals; has a fallback for React v15).
  2. Refactor code base for Modal component.
  3. Implement better fix for #761.

Can I take a look at this?

cc @TheSharpieOne @martindavid

@TheSharpieOne

This comment has been minimized.

Member

TheSharpieOne commented Jan 29, 2018

@fantua go for it

@supergibbs supergibbs added this to To Do in v5.0.0 via automation Jan 31, 2018

@supergibbs supergibbs removed this from To Do in v5.0.0-beta Jan 31, 2018

v5.0.0 automation moved this from To Do to Done Feb 12, 2018

jlaramie added a commit to jlaramie/reactstrap that referenced this issue Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment