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

[changed] Don't render div on dom when isOpen is false #205

Merged
merged 1 commit into from Jul 2, 2017

Conversation

CompuIves
Copy link
Contributor

I noticed that when I wanted to animate the fadein of a modal it only worked for the first time. This was because the css animation of ReactModal__Overlay--after-open only works when a div is rendered for the first time. That's why I changed the logic to render nothing if there is no modal.
I'm aware that this can break a lot of things (like using a ref for ReactModal) for other people, so maybe we need to bump a major version if we want to have this.

Changes proposed:

  • When not open, render null instead of div

Upgrade Path (for changed or removed APIs):
When you want to use a ref on ReactModal and use it after it's rendered, you will have to place the call to the ref in a setTimeout so it's accessible after it's rendered.

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.

This allows users to add an animation for opening the modal.

@diasbruno
Copy link
Collaborator

@claydiffrient any thoughts? looks good.

@claydiffrient
Copy link
Contributor

@CompuIves Sorry for the delay on this. Could you change the commit message from [fixed] to [changed] ? I think that will make it easier to indicate that it's more of a breaking change vs. a simple bug fix. I believe this is a valuable change. I'd like to get it in for v2.

@claydiffrient claydiffrient added this to the 2.0.0 milestone Nov 1, 2016
@CompuIves CompuIves changed the title [fixed] Don't render div on dom when isOpen is false [changed] Don't render div on dom when isOpen is false Nov 1, 2016
@CompuIves
Copy link
Contributor Author

No problem! I changed it.

@diasbruno diasbruno modified the milestones: 1.7, 2.0.0 Mar 2, 2017
@yvele yvele mentioned this pull request Mar 7, 2017
@diasbruno diasbruno modified the milestones: 1.7, 1.8 Jun 12, 2017
@diasbruno
Copy link
Collaborator

Hi @CompuIves, we have released v2.0.0. This will need a rebase.

@CompuIves
Copy link
Contributor Author

Okay! Will do it today.

@CompuIves
Copy link
Contributor Author

There were a lot of changes, I decided to just recreate the changes based on new master.

I wasn't able to reproduce the rendering of div instead of null in the tests anymore, maybe I'm doing something wrong or something has changed.

@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage decreased (-13.5%) to 71.429% when pulling dc14e2d on CompuIves:master into 8f3898a on reactjs:master.

@diasbruno
Copy link
Collaborator

diasbruno commented Jun 20, 2017

Awesome, @CompuIves!
Perhaps after close the modal you can check if modal.portal == null.

@CompuIves
Copy link
Contributor Author

Ah, that works, thanks! Added test.

@diasbruno
Copy link
Collaborator

yeah, that is correct.

@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage remained the same at 84.956% when pulling 9f3fd85 on CompuIves:master into 8f3898a on reactjs:master.

@diasbruno
Copy link
Collaborator

diasbruno commented Jun 20, 2017

You can squash the commits and append [chore] at the begging of the commit message and it is good to merge.

@CompuIves
Copy link
Contributor Author

Done!

@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage remained the same at 84.956% when pulling 1639de8 on CompuIves:master into 8f3898a on reactjs:master.

@diasbruno diasbruno merged commit e56c414 into reactjs:master Jul 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants