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

Reuses MarkerClusterGroup #647

Merged
merged 5 commits into from
May 26, 2020
Merged

Reuses MarkerClusterGroup #647

merged 5 commits into from
May 26, 2020

Conversation

derekmiranda
Copy link
Member

MarkerClusterGroup seems to extend LayerGroup so should be fine using
backend-determined clusters while this plugin handles stacked pins. Not sure if this is a regression since @jmensch1 had changed it from MarkerClusterGroup to LayerGroup.

Also, pins at same lat/lng don't seem to automatically spiderfy even though spiderfyOnMaxZoom is enabled by default. Users will have to click on a fully zoomed-in stacked cluster to spread them.

Fixes #{issue number here}

  • Up to date with dev branch
  • Branch name follows guidelines
  • All PR Status checks are successful
  • Peer reviewed and approved

Any questions? See the getting started guide

MarkerClusterGroup seems to extend LayerGroup so should be fine using
backend-determined clusters while this plugin handles stacked pins
@jmensch1
Copy link
Contributor

Hey @derekmiranda, thanks for the help!

I don't think this is necessarily a regression, since I wasn't thinking of the stacked pins problem when I took out the MarkerClusterGroup originally. If it solves that problem, them I'm all for it.

However, I did notice a different bug that we had before with the MarkerClusterGroup. When you're zoomed all the way in and click on a cluster with stacked pins, they do the spiderify thing. And then if you click on one of the pins, the popup appears for a moment and then disappears. If you click on the cluster again, and then click on the same pin again, it stays open.

I'm not sure if there's a way to fix that. Maybe it should be a new ticket.

Copy link
Member

@adamkendis adamkendis left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Agree with what Jake said - no issues with reusing MarkerClusterGroup if it accomplishes what we need. Unfortunately, it did reintroduce the bug in #461 . I reopened the bug ticket in case you wanted to take a look at it.

@adamkendis adamkendis self-requested a review May 26, 2020 22:33
@adamkendis
Copy link
Member

Even with the autoclustering after clicking a pin, this is better than stacked pins. Going to merge this and we can tackle the clustering in another PR.

@adamkendis adamkendis merged commit 0fffe18 into dev May 26, 2020
@adamkendis adamkendis deleted the 591_FE_spreadStackedPins branch May 26, 2020 22:34
@derekmiranda
Copy link
Member Author

Hey guys, thanks for reviewing! I'll take a look at the reopened issue

@adamkendis adamkendis mentioned this pull request May 27, 2020
1 task
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