-
Notifications
You must be signed in to change notification settings - Fork 812
Deprecate isOpen
#25
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
Comments
I've also found it to be more convenient this way |
I think you'd lose out on being able to animate? I don't think you can wrap |
@pstoica Yea. I just realized this. Closing this because I just made myself look stupid 🎉 haha |
@pstoica @srph @monsieurnebo @diasbruno So basically the whole point of If I don't have animation, can you confirm that no using What the point of the portal? Only animation or is there some a11y magic? Related to #205 |
The @yvele Can you elaborate your idea to handle this? Resources: |
@yvele @diasbruno i modified the library a bit and not using isOpen works fine but then you lose out on animation. The reason I did it is because if i use the library, as is, I can't get react-router to work properly where i render a component based on a route. In my app the modals have urls and it seems react-modal is designed to be mounted when the page loads and show or hides the modal based on the isOpen prop instead of mounting and unmounting the whole component based on a route. I think it'd be better to get rid of the isOpen prop but i don't know how you'd handle animation then. I was not able to figure that part out, but i imagine there must be some other way to handle animations. |
I believe i've been able to get it working without having to remove isOpen with react-router by using the children function: That way the component mounts no matter what and the modal can continue using isOpen with animation. |
I find this more convenient compared to the current approach.
However, I haven't tried this (whether is works), and I do not acknowledge any pros and cons of the approach besides a cleaner API.
The text was updated successfully, but these errors were encountered: