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

v3 don't respect closeTimeoutMS on unmount anymore #530

Closed
Kerumen opened this issue Oct 9, 2017 · 14 comments
Closed

v3 don't respect closeTimeoutMS on unmount anymore #530

Kerumen opened this issue Oct 9, 2017 · 14 comments

Comments

@Kerumen
Copy link

Kerumen commented Oct 9, 2017

Summary:

Before the React 16 compatibility, we were able to unmount the modal with an animation. It's not possible anymore with the v3.

Steps to reproduce:

  1. Add a <Modal /> with closeTimeoutMS={200}
  2. Tweak the CSS to display an open and a close animation
  3. Mount/unmount the modal

Expected behavior:

The Modal should unmount with an animation.

Link to example of issue:

React 15 and v2: https://codesandbox.io/s/7k63p8lk81 (working)
React 16 and v3: https://codesandbox.io/s/mml6ql88py (not working)

@diasbruno
Copy link
Collaborator

Hi @Kerumen, prefer to using isOpen instead of conditional render for modal for now. Need to check when modal is forcible unmounted case.

@diasbruno diasbruno added the bug label Oct 9, 2017
@Kerumen
Copy link
Author

Kerumen commented Oct 9, 2017

Yes I know. In my case I can't really use isOpen.
And as it used to work, I report this as a regression. :)

@diasbruno
Copy link
Collaborator

On src/components/Modal.js there is a setTimeout. Can you dig in a little bit (if you have time)?

@Kerumen
Copy link
Author

Kerumen commented Oct 9, 2017

I tried but no luck. Event with the line commented out the modal seems to unmount instantly.
Maybe this is a change on how React handle willUnmount?

@diasbruno
Copy link
Collaborator

Possibly. I'll try something this evening.

@TeamChad
Copy link

Hey @diasbruno. Is there any movement on this bug? We are really keen to upgrade our project to React 16 and this is the only issue holding us up :-) We are keen to keep using this great library!

@diasbruno
Copy link
Collaborator

diasbruno commented Nov 27, 2017

@TeamChad Can you do some debugging on this issue? I didn't have time yet to have look at this one (sorry), but I can give some help...One place to look at is the documentation of createPortal.

@diasbruno
Copy link
Collaborator

diasbruno commented Nov 28, 2017

Still didn't find a solution for this issue. We can start a discussion about "why using conditional render for a modal?". Just to get some information/use case for this. cc @TeamChad @Kerumen.

@chriswetterman
Copy link

@Kerumen If you change your rendering of the modal to what's below this will solve the issue in your "not working" codesandbox.io link. I had used conditional rendering and needed to make this change after updating React.

    const { modal } = this.state;
    return (
      <div>
        <button onClick={() => this.toggleModal()}>
          Open Example modal
        </button>
        <Modal
          closeTimeoutMS={200}
          isOpen={this.state.modal}
          contentLabel="modal"
          onRequestClose={() => this.toggleModal()}
        >
          <h2>Add modal content here</h2>
        </Modal>
      </div>
    );
  }

@hoodwink73
Copy link
Contributor

Hello folks

I did some debugging. Finally!

Here's what I found. React 16 does not provide any control over unmounting the portal. The portal unmounts whenever the parent unmounts.

facebook/react#10143 (comment) – this issue clarifies that

The issue further points out that the ability to control the unmounting of portal with unstable_renderIntoContainer API(React 15) was never a feature. And it introduced problems for orphaned portal subtrees during their subsequent update cycles.

So, henceforth we will not be able to control the unmounting of the portal using the createPortal API.

Now lets take a look at React Modal code. I will present what I understand after the debugging.

componentWillUnmount() {
if (!canUseDOM || !this.node || !this.portal) return;
const state = this.portal.state;
const now = Date.now();
const closesAt =
state.isOpen &&
this.props.closeTimeoutMS &&
(state.closesAt || now + this.props.closeTimeoutMS);
if (closesAt) {
if (!state.beforeClose) {
this.portal.closeWithTimeout();
}
setTimeout(this.removePortal, closesAt - now);
} else {
this.removePortal();
}
}

In case of React 15, unmounting the parent will set up a timer. And after the timer elapses, removePortal is invoked. And it unmounts the portal using ReactDOM.unmountComponentAtNode. And this triggers the portal’s componentWillUnmount.

removePortal = () => {
!isReact16 && ReactDOM.unmountComponentAtNode(this.node);
const parent = getParentElement(this.props.parentSelector);
parent.removeChild(this.node);
};

In React 15, the fading out animation of the portal takes place between these componentWillUnmount of the parent(of portal) and removePortal, during which the portal very much stays in the DOM.

In React 16, we lose the ability to schedule the unmounting of the portal itself. Hence we cannot perform any animation.

@jcrben
Copy link

jcrben commented Nov 8, 2018

Odd. I'm able to fade out without any issue. Running:

  • "react-modal": "^3.6.1"
  • "react-dom": "^16.4.2"
  • "react": "^16.4.2"

and basically just using http://reactcommunity.org/react-modal/styles/transitions.html

@befront
Copy link

befront commented Nov 19, 2018

Somebody find the solution for closing animation?

@diasbruno diasbruno added this to the 4.0 milestone Feb 14, 2019
@nextlevelshit
Copy link

I am using react-modal with GatsbyJS. The code is bit oversized, but you will find what you (@befront ) need in the hideModal() function:

import React from "react"
import ReactModal from "react-modal"
import { navigate } from "gatsby"

import "./modal.component.scss"

/**
 * Necessary implementation for screen-readers. Read more:
 * http://reactcommunity.org/react-modal/examples/set_app_element.html
 */
ReactModal.setAppElement(`#___gatsby`)

class Modal extends React.Component {

  classes = {
    overlayClassName: {
      base: `modal__backdrop`,
      afterOpen: `modal__backdrop--after-open`
    },
    className: {
      base: `modal__inner`,
      afterOpen: `modal__inner--after-open`
    },
    bodyOpenClassName: `modal__body--open`
  }

  options = {
    closeTimeout: 660
  }
  
  cssVariables = {
    // Necessary CSS variable for modal transition
    //
    '--modal-transition-duration': `${this.options.closeTimeout}ms`,

    // Optional CSS variables to adjust modal behaviour
    //
    // '--modal-overlay-color': `255, 255, 255`,
    // '--modal-overlay-alpha': `0.9`,
    // '--modal-timing-function': `cubic-bezier(.86 ,0, .32, 1)`,
    // '--modal-body-shift': `-10vw`;
  }

  constructor() {
    super()
    this.state = {
      showModal: false
    }
  }

  componentDidMount() {
    this.applyCssVariables()
    this.showModal()
  }

  componentWillUnmount() {
    this.removeCssVariables()
  }

  render() {
    const { children } = this.props
    const { showModal } = this.state
    const { closeTimeout } = this.options

    return (
      <ReactModal
        isOpen={showModal}
        onRequestClose={() => this.hideModal()}
        closeTimeoutMS={closeTimeout}
        {...this.classes}
      >
        <div className="container">
          {children}
        </div>
      </ReactModal>
    )
  }
  /**
   * Changes state to opened modal.
   */
  showModal() {
    this.setState({
      showModal: true
    })
  }
  /**
   * changes state to closed modal and navigates to the index page.
   */
  hideModal() {
    const { closeTimeout } = this.options
    const { bodyOpenClassName, className, overlayClassName } = this.classes

    document.body.classList.remove(bodyOpenClassName)
    document.querySelector(`.${className.base}`).classList.remove(className.afterOpen)
    document.querySelector(`.${overlayClassName.base}`).classList.remove(overlayClassName.afterOpen)

    setTimeout(() => {
      this.setState({
        showModal: false
      })
      navigate(`/`)
    }, closeTimeout)

    // 
  }
  /**
   * Applys all CSS Variables to the document so the stylesheet
   * can use certain variables required for the modal behaviour.
   */
  applyCssVariables() {
    Object.keys(this.cssVariables).forEach(key => {
      const value = this.cssVariables[key]
      return document.documentElement.style.setProperty(key, value)
    })
  }
  /**
   * Removes all CSS variables in order not to spoil the DOM unnecessarily.
   */
  removeCssVariables() {
    Object.keys(this.cssVariables).forEach(key => {
      const value = this.cssVariables[key]
      document.documentElement.style.removeProperty(key, value)
    }) 
  }
}

export default Modal

If you're having questions, let me know. I will explain the code.

@diasbruno
Copy link
Collaborator

I'm closing this one. It looks like a bug, but actually is how the new behavior of createPortal.

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

No branches or pull requests

8 participants