reactjs / react-modal

Accessible modal dialog component for React
http://reactcommunity.org/react-modal
MIT License
7.37k stars 809 forks source link

Question: Unmounting the parent & the beforeClose call #459

Open declanelcocks opened 7 years ago

declanelcocks commented 7 years ago

Summary:

I have a page, with some components that are conditionally rendered as below and some of these components will contain a modal. The modals themselves have no internal state or anything like that.

If you check below, this is the onClose which eventually gets applied to the modal. So the hideModal function is getting called, hiding the modal and even removing any aria attributes on the body. Good πŸ‘

However, the setTab part will cause my conditional render to destroy the parent that the modal is inside. As a result of this, the ReactModal__Body--open class is never removed from the body πŸ€” It will remove this class if I do any of the following:

What is the recommended approach for something like this? It's very odd that the modal itself and the aria attribute are removed, but the body class is left behind. It all stems from the fact the parent is conditionally shown.

Link to example of issue:

{showTab === 1 && <TabOne />}
withHandlers({
    onClose: ({ setTab, hideModal }) => () => {
        setTab(2);
        hideModal();
    }
}),
diasbruno commented 7 years ago

@declanelcocks Hi, can write a small app for reproducing this issue? You can use the react-modal bootstrap on https://codesandbox.io/s/mgwy6V6E.

declanelcocks commented 7 years ago

@diasbruno https://codesandbox.io/s/npjoXq1W I'm not sure that this is 100% replicating my situation, but you can see the class is never removed from body. Basically the call to close the modal is also destroying its parent, which causes the modal to be destroyed before the class is removed.

To illustrate my point, if you wrap the toggleContainer call in a setTimeout and you'll see that the class is removed from the body successfully.

diasbruno commented 7 years ago

Perhaps, when closeTimeoutMS is defined and the modal is destroyed, it doesn't do a proper close action. Any extra logs from react?

declanelcocks commented 7 years ago

@diasbruno No, nothing else from react. You're also right that if I remove the closeTimeoutMS then the body class is successfully removed even after the modal is destroyed. For now, the most convenient fix I've found is to wrap all the "extra" requests (like setTab in my OP) in a setTimeout as it allows me to still structure my components logically.

diasbruno commented 7 years ago

Another way to solve this is to move the modal component out of the tab and notify when to open and close with callbacks or events, something like:

class Controller ... {
   xhrAndRenderModalWithCollection1 () { ... this.Renderer = Renderer1; }
   xhrAndRenderModalWithCollection2 () { ... this.Renderer = Renderer2; }

   render() {
       const { Renderer } = this;
       <div>
           <tabs>
               {showTab === 1 && <TabOne onAction={this.xhrAndRenderModalWithCollection1} />}
               ...
           <tabs>
           <modal isOpen ...>
               <Renderer> ... </Renderer>
           </modal>
       </div>
    }
}

It seems better to construction this way, because it creates a proper controller to manage a Route, for example, and use the same modal for rendering.

declanelcocks commented 7 years ago

Move the rendering of the modal to outside of this conditional render (no problem since isOpen is decided be redux anyway)

@diasbruno Yeah that was one "fix" I mentioned in my OP that I considered. The modal will always be called by this tab, so it makes sense to have the <Modal /> component inside that tab. Using a setTimeout is better for me, as it means I don't have a random <Modal /> rendered somewhere in my code.

diasbruno commented 7 years ago

@declanelcocks that's fine.

I'll check if there is something react-modal can do. If we get Modal::componentWillUnmount() to be fired on this conditional render, we can move back beforeOpen() and beforeClose() from ModalPortal.js to Modal.js.

declanelcocks commented 7 years ago

There are workarounds, but the great thing about React is that components can be completely isolated. This bug it makes it difficult to do that without some workarounds, so thanks for looking into it @diasbruno πŸ‘

ttbarnes commented 7 years ago

I've had a similar issue with multiple tooltips that utilise react-modal along with a parentSelector. Adding a small timeout to each open/close/toggle method fixes it.