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 closeModal invoked right after openModal when used in stencilJS Web Component with Maplibre #22

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

Conversation

lbrutti
Copy link

@lbrutti lbrutti commented Apr 12, 2022

see issue #21

@el
Copy link
Owner

el commented Apr 13, 2022

Hi, Thanks for taking the time to create a pr, however, this would mean a difference in functionality. we don't want to only listen the container. Maybe it can be a fallback instead?

lib/index.ts Outdated
styleElement.classList.add("active");
}
this.mapStyleContainer.appendChild(styleElement);
}
this.styleButton.classList.add("mapboxgl-ctrl-icon");
this.styleButton.classList.add("mapboxgl-style-switcher");
this.styleButton.addEventListener("click", event =>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please dont update the coding style of the project

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ops, sorry, I hope to have reverted to original code style now

@lbrutti
Copy link
Author

lbrutti commented Apr 13, 2022

hello :) thanks for the replies.

I added an optional flag in the constructor to bind the click event to the mapContainer if is set to true. do you think this could be a good approach?

Thanks!!!

@el
Copy link
Owner

el commented Apr 25, 2022

I think a better approach would be to detect if the code is running inside a shadow root or not and bind accordingly. Sorry, it took a while for me to respond, I had covid last week.

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