-
-
Notifications
You must be signed in to change notification settings - Fork 96
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
Add keepMounted option and fix screen glitch #516
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pr! Could you please add an example in the docs so I can try it? |
.react-responsive-modal-overlay, | ||
.react-responsive-modal-container, | ||
.react-responsive-modal-modal { | ||
animation-fill-mode: forwards !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please explain why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes a separate issue #495. I copied the solution from one of the people in that thread. To be honest, I'm not sure why it helps, but it does.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #516 +/- ##
==========================================
+ Coverage 95.28% 95.45% +0.16%
==========================================
Files 6 6
Lines 191 198 +7
Branches 69 74 +5
==========================================
+ Hits 182 189 +7
Misses 9 9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Hey @pradel, thanks for the response. I added an example and docs in the Next project. I also changed how modals are centered, from the "inline" behavior to flex-box centering. I did this because there was an issue when the modal had little space (e.g. on a mobile device) and the pseudo-element you had before was wrapped to a new line. This implementation doesn't do that. |
@@ -103,6 +104,26 @@ You can also set to trap focus within the modal, but decide where to put focus w | |||
|
|||
``` | |||
|
|||
### Keeping a hidden modal mounted | |||
|
|||
By setting the `keepMounted` property to `true`, you can specify that the modal should always be mounted to the DOM, even when hidden it is hidden. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a small typo:
By setting the `keepMounted` property to `true`, you can specify that the modal should always be mounted to the DOM, even when hidden it is hidden. | |
By setting the `keepMounted` property to `true`, you can specify that the modal should always be mounted to the DOM, even when it is hidden. |
@pikaju could you please split the pr into two parts?
|
@pradel Sorry, but that doesn't seem worth the effort, I suggest just releasing a new major version |
Adds option that keeps the Modal mounted even when it is hidden. This is useful for keeping the DOM state inside the Modal as well as for SEO purposes.
Fixes #233 and #495