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

Update MarkerClusterGroup.js #609

Merged
merged 1 commit into from
Dec 7, 2015

Conversation

OriginalSin
Copy link
Contributor

Use this._maxZoom instead map.getMaxZoom() in _zoomOrSpiderfy.
When we use disableClusteringAtZoom in options this._maxZoom not equal map.options.maxZoom.

Use this._maxZoom instead map.getMaxZoom() in _zoomOrSpiderfy.
When we use disableClusteringAtZoom in options this._maxZoom not equal map.options.maxZoom.
@ghybs
Copy link
Contributor

ghybs commented Nov 30, 2015

Hi!

You are right, it previously used map.getMaxZoom() === map.getZoom() and I just re-used map.getMaxZoom().

But since I merged the cases when all markers are at the exact same position and when we are at maxZoom, I accidentally removed spiderfication of markers at the exact same position when disableClusteringAtZoom is used with a lower zoom than map.options.maxZoom, as you point out.

For the case of markers at "slightly different positions", it could still be interesting to zoom rather than spiderfy, because the map has more zoom levels to separate out those markers. But they will not necessarily separate a lot!

So spiderfying with checking this._maxZoom rather than map.options.maxZoom sounds a reasonable choice, especially with the fact that application developer can still tune the behaviour with spiderfyOnMaxZoom option.

danzel added a commit that referenced this pull request Dec 7, 2015
@danzel danzel merged commit d9c1310 into Leaflet:master Dec 7, 2015
@danzel
Copy link
Member

danzel commented Dec 7, 2015

Cool 👍

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