Skip to content

Conversation

mattiasb
Copy link
Contributor

This code makes removeLayer and addLayer emit a zoomlevelschange-event if the zoomlevels were changed as a result of adding / removing the layer. Tests are included.
Please consider merging this also to stable (for 0.5.2) since I plan on making use of this in the zoom slider plugin (tell me if I should make a separate PR for that.

This is triggered when you remove a layer from a map with greater
zoom level coverage than the remainding layers or when you add a tilelayer
with greater zoomlevel coverage than the previous set of layers had.
@@ -271,6 +271,10 @@ L.Map = L.Class.extend({
return Math.min(z1, z2);
},

getZoomLevels: function () {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this method should be private (starting with _), I don't see any applications externally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a copy of that method in the zoomslider plugin and when I saw that I needed it here too I thought "why not just expose it?". It's just a one liner though so doesn't matter much to me one way or another. :)

Copy link
Member

Choose a reason for hiding this comment

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

The less unessential methods API exposes the better, so lets still make it private.
And maybe change the name, as it suggests more of an array result, to something like _getZoomSpan or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, agree! I'll go for _getZoomSpan, unless you actually prefer just doing:

var zoomSpan = this.getMaxZoom() - this.getMinZoom();

inside _updateZoomlevels even?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above didn't make sense. Forgot i called getZoomSpan twice. Added a commit that fixes this just now.

@mourner
Copy link
Member

mourner commented Feb 14, 2013

Stable version usually only gets critical bugfixes and regression fixes, so I'd leave that up to master.
You can branch out the zoomslider for stable version and then make changes for the master Leaflet version on your master branch.

@mattiasb
Copy link
Contributor Author

That sounds reasonable. :)

@mourner
Copy link
Member

mourner commented Feb 14, 2013

Looks good!

mourner added a commit that referenced this pull request Feb 14, 2013
@mourner mourner merged commit a948379 into Leaflet:master Feb 14, 2013
@mourner
Copy link
Member

mourner commented Feb 14, 2013

Make a docs pull too.

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