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: check if overlay was already instantiated before creating a new one #571

Merged
merged 6 commits into from
Jan 16, 2024

Conversation

silvester-pari
Copy link
Collaborator

@silvester-pari silvester-pari commented Jan 15, 2024

This PR goes in a different direction than #555: when the select interaction is initialized, first check if an eox-map-tooltip was already instantiated on the map, if so, reuse that overlay, if not, create a new one.
This should fix the issue with not finding the DOM element, but means only one tooltip can exist per map.

@silvester-pari silvester-pari marked this pull request as ready for review January 15, 2024 16:00
Copy link
Contributor

@RobertOrthofer RobertOrthofer left a comment

Choose a reason for hiding this comment

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

While this does still seem a little hacky to me, i like the approach of going back to having a single tooltip element for the entire map.
This disregards tooltips for click selection, which is ok I suppose for the current use cases and the current API.

@silvester-pari
Copy link
Collaborator Author

While this does still seem a little hacky to me, i like the approach of going back to having a single tooltip element for the entire map. This disregards tooltips for click selection, which is ok I suppose for the current use cases and the current API.

Thanks for your review. While this indeed "breaks" the functionality to have e.g. different tooltips on hover and on click, the basic tooltip functionality should be more robust without introducing breaking changes to the API.

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.

3 participants