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

[fixed] Clear the delayed close timer when modal opens again #134

Closed
wants to merge 1 commit into from

Conversation

CompuIves
Copy link
Contributor

Currently there is an inconsistency when there is a delayed close and right after the start of this close the modal opens again.
Graphic:

| closeWithTimeout
|   setState - beforeClose: true
| open
|   setState - isOpen: true
|   setState - afterOpen: true
| closeWithoutTimeout
|   setState - beforeClose: false
|   setState - afterOpen: false
v

Now we have this state:

{
  beforeClose: false,
  isOpen: true,
  afterOpen: false,
}

Which gives an invisible modal.

This PR should fix it by just skipping the open logic (since the modal is already open) and clearing the timeout and returning beforeClose to false.

@claydiffrient
Copy link
Contributor

@CompuIves Any chance you'd be willing to put a spec in place with this? It looks like a pretty good change, but I'd prefer that changes have associated specs with them.

@diasbruno
Copy link
Collaborator

Pinging @CompuIves. This seems right. Can you fix the conflict and add a spec for this?
I'll test it.

@CompuIves
Copy link
Contributor Author

Ah, I completely missed the first reply, sorry! I'm currently travelling in SE-Asia without any access to my macbook. I can create a spec when I come back (May 26th) but I'm not sure if you would like to wait so long.

@diasbruno
Copy link
Collaborator

@CompuIves nice. Do you mind if I jump in this fix?

@CompuIves
Copy link
Contributor Author

@diasbruno Sure, no problem!

@diasbruno
Copy link
Collaborator

@CompuIves thanks. I'll keep a reference to your work on it.

@diasbruno
Copy link
Collaborator

yep, almost forgot...i'll finish this today!

@CompuIves
Copy link
Contributor Author

Hi! What's the progress of this fix?

diasbruno added a commit to diasbruno/react-modal that referenced this pull request May 31, 2016
@diasbruno
Copy link
Collaborator

@CompuIves I've rebased the branch and added the test to it. I didn't had time to work on this, but I believe it's finished (almost).

You can check it here...
diasbruno/fix/clearTimeout

diasbruno added a commit to diasbruno/react-modal that referenced this pull request Jun 23, 2016
diasbruno added a commit to diasbruno/react-modal that referenced this pull request Jun 23, 2016
diasbruno added a commit to diasbruno/react-modal that referenced this pull request Jun 23, 2016
@diasbruno
Copy link
Collaborator

@claydiffrient this PR can be closed in favor of #189.

@diasbruno
Copy link
Collaborator

This PR can be closed in favor of #189.

claydiffrient pushed a commit that referenced this pull request Jun 30, 2016
* [fixed] Clear the delayed close timer when modal opens again

* [fixed] added test for the timeout case.

continuation of the PR #134.

thanks, @CompuIves.
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.

3 participants