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

Fix close button target when using SVG as inner content #503

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mtscarvalho
Copy link

When some SVG tag is added inside the close button tag, the action of closing the modal doesn't work. This PR aims to fix that problem.

@vercel
Copy link

vercel bot commented Oct 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
micromodal ❌ Failed (Inspect) Oct 6, 2022 at 4:05PM (UTC)

@kalpeshsingh
Copy link
Collaborator

Hello @mtscarvalho 👋
Thanks for spotting a bug and sending a PR to fix it.

Wondering why do you need to explicitly use pointer-events? The default auto should work?

@mtscarvalho
Copy link
Author

Hello, @kalpeshsingh ! 👋

When you click on the SVG, the event target is no longer the button element, but the path inside the SVG or the svg element itself. As a consequence, the event target fails and the closing action does not happen.

A way to fix that is disabling the event fires on SVG. There 2 ways about how to do that:

  • Assigning the pointer-events as none to SVG
  • Assigning the pointer-events as all for the parent element, in this case the button.

The auto value should work for all cases with the exception of SVG.

@mtscarvalho
Copy link
Author

Hello @kalpeshsingh!

What do you think of this fix? It's my first interaction and PR with an open-source project. Please, let me know if I did something wrong or if you have some suggestions.

@kalpeshsingh
Copy link
Collaborator

Hello @mtscarvalho I will let @ghosh make a decision here.

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.

2 participants