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] add stopPropagation at handleOverlayOnClick #940

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joaoGabriel55
Copy link

Fixes #939.

Changes proposed:

  • Add stopPropagation at handleOverlayOnClick

Upgrade Path (for changed or removed APIs):

  • None

Acceptance Checklist:

  • 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.

@@ -285,6 +285,8 @@ export default class ModalPortal extends Component {
};

handleOverlayOnClick = event => {
event.stopPropagation();

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.

Copy link
Author

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

Copy link
Collaborator

@diasbruno diasbruno Apr 25, 2022

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.

Copy link
Author

@joaoGabriel55 joaoGabriel55 Apr 25, 2022

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

Copy link
Collaborator

@diasbruno diasbruno Apr 27, 2022

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.

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.

@TheRakeshPurohit
Copy link

This needs to be merged and released if this allows handling overlay click events.

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.

handleOverlayOnClick is not stopping click event propagation
4 participants