[fixed] respect closeTimeoutMS during unmount

[mgh]

Fixes #17.

Changes proposed:

• wait before unmounting the portal if closeTimeoutMS is set
• full details in commit message

Upgrade Path (for changed or removed APIs):

• The upgrade path is to undo any workarounds or hacks (e.g. manually triggering the close animation before unmounting the Modal), though due to the lack of activity on the referenced issue, I'm guessing most users won't be affected?

Acceptance Checklist:

• All commits have been squashed to one.
• The commit message follows the guidelines in CONTRIBUTING.md.
• Documentation (README.md) and examples have been updated as needed.
• If this is a code change, a spec testing the functionality has been added.
• If the commit message has [changed] or [removed], there is an upgrade path above.

[mgh]

if the Modal is unmounting, it won't have a chance to update the isOpen prop to false; however, the internal setState call will update state.isOpen correctly.

[claydiffrient]

@mgh This looks good from a code perspective, but it'd be great to have a spec in place. I think we could do an async spec that verified that the time the callback is called is within some small margin of error from the timeout value specified.

[coveralls]

Coverage increased (+0.7%) to 77.739% when pulling a3e079c on mgh:master into 6ba1c8d on reactjs:master.

[mgh]

@claydiffrient sounds good. I've updated the test specs to include an async spec as you proposed. The new test queries the DOM after the modal is unmounted to ensure the portal still exists until the closeTimeoutMS expires, so I had to update the other tests to ensure other modals weren't still lying around. (I noticed a related test for closeTimeoutMS that was commented out because it didn't work, so I fixed that as well.)

[diasbruno]

@mgh awesome! Is this action cancelable? (Just a question).

[mgh]

@diasbruno depends on what you mean by cancelable =) the original behavior of closing and then immediately reopening a modal should be unaffected (the first close is canceled and the portal is preserved). Unmounting then remounting the same Modal will not preserve the same portal though, as React instantiates a new Modal instance for each mount and each instance has its own portal. That behavior, though, is the same before and after this commit.

[diasbruno]

Rebase. 👍

[diasbruno]

I thought If I approve this I could rebase from the interface. :)

[mgh]

@diasbruno I rebased. Two things to note:

1. Original change (ebec638#diff-9e8d9aa8af75942f5423a4b030c5b78aR145):

   this.setState({
      ...                                                   
   }, this.afterClose);          →        }, () => this.afterClose());
   

   However, this same commit redefined afterClose:

   afterClose = () => {
   

   So I kept the original }, this.afterClose) since the extra function wrapper does not appear necessary.

2. A new test did not unmount the modal, breaking my test which expects no modals to be left-over from previous tests. This same problem may affect tests in the future, and it's hard to debug since adding a new test can break my (unrelated) test. Thus, in addition to fixing the test in question, I also added:

   afterEach('check if test cleaned up rendered modals', () => { ... });
   

   which will now correctly identify tests that don't clean up:

   ...
   ✔ give back focus to previous element or modal.
   ✖ "after each" hook: check if test cleaned up rendered modals for "give back focus to previous element or modal."
   
   Finished in 1.446 secs / 0.072 secs
   
   SUMMARY:
   ✔ 11 tests completed
   ℹ 2 tests skipped
   ✖ 1 test failed
   
   FAILED TESTS:
     Modal
       ✖ "after each" hook: check if test cleaned up rendered modals for "give back focus to previous element or modal."
   

   Arguably, the afterEach hook could try automatically unmounting all modals first to make test writing easier.

[coveralls]

Coverage increased (+0.2%) to 80.702% when pulling ac01dda on mgh:master into ebec638 on reactjs:master.

[coveralls]

Coverage increased (+0.5%) to 81.053% when pulling 34927f1 on mgh:master into ebec638 on reactjs:master.

[diasbruno]

Yeah, I've added this wrapper on afterClose (my bad) and forgot to remove. I add this because It was not firing cause it was a class method and not instance method or something.

Really like the afterEach, it is the best place to do some clean up for the next test. I believe we should add a function to force unmount any remain modal after a test.

[diasbruno]

Tested and working.