-
Notifications
You must be signed in to change notification settings - Fork 808
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] add stopPropagation at handleOverlayOnClick #940
base: master
Are you sure you want to change the base?
[changed] add stopPropagation at handleOverlayOnClick #940
Conversation
src/components/ModalPortal.js
Outdated
@@ -285,6 +285,8 @@ export default class ModalPortal extends Component { | |||
}; | |||
|
|||
handleOverlayOnClick = event => { | |||
event.stopPropagation(); |
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.
I came across this pull request as my events aren't propagating right now when I want them to (I'm doing something wrong in my app). My scenario is that I have a transparent overlay and shouldCloseOnOverlayClick
is set. I want the modal to close and underlying control to be clicked.
I don't think that event.stopPropagation()
should always happen; it should be opt-in (or a setting to control the behavior). With this change, I believe it's possible to break existing use of react-modal
that relies on this behavior.
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.
Thanks for the review @smcenlly. I will take a look
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.
We can add the stopPropagation there...we need to be careful here to only stop if the event target is the overlay.
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.
I am checking this, in this commit: 4dd8ee3
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.
Muito bem...um potiguar por aqui!!
I think we are close to get this done...can this event
not be related to the overlay? I'm not sure if that is a valid case, but it would be nice to check it.
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 needs to be merged and released if this allows handling overlay click events.
This needs to be merged and released if this allows handling overlay click events. |
Fixes #939.
Changes proposed:
Upgrade Path (for changed or removed APIs):
Acceptance Checklist:
CONTRIBUTING.md
.