neveldo / jQuery-Mapael

jQuery plugin based on raphael.js that allows you to display dynamic vector maps
https://www.vincentbroute.fr/mapael/
MIT License
1.01k stars 195 forks source link

min level set #210

Closed okwme closed 8 years ago

okwme commented 8 years ago

I'm sorry I didn't understand what you meant before when you said I should set

self.options.map.zoom.init = self.options.map.zoom.minLevel 

if minLevel is set.

Indigo744 commented 8 years ago

Thanks for this PR.

Just to be sure, this modification allows the user to specify a min level different from 0, right?

neveldo commented 8 years ago

That's it @Indigo744 .

Here is a JSFiddle where I put the updated code from @billyrennekamp : http://jsfiddle.net/neveldo/uz72posg/ .

Actually, it works fine. But you can see in the JSFiddle that I have set minLevel to 5, and, the map is initialized with a zoomLevel = 0 (no zoom, this is the default behavior). So, when the user click on the '+' button or use the mousewheel to zoom-in, it will set the zoomLevel from 0 to 5 (the minLevel value). Maybe it's not an issue because the user is able to set an initial zoom level through the options in order to avoid this behaviour.

So I'm still wondering if we should force the initial zoomLevel to the minLevel value if set or no ... What do you think about it ?

Indigo744 commented 8 years ago

I see! This is because the zoom event is only triggered at creation if self.options.map.zoom.init is set (see line 213).

This can be easily corrected by adding a default options options.map.zoom.init like this:

defaultOptions: {
    ...
    zoom: {
        ...
        init: {
            level: 0
        },
        ...
    }
    ...

That way, zoom event is always triggered and the minimum value will be applied.

neveldo commented 8 years ago

Indeed, it works fine by setting a default value for the initial zoom level : http://jsfiddle.net/neveldo/gqzgezjf/ . However, by doing this, all the instances will run all the code within onZoomEvent() function, just to avoid a little side effect with the ongoing minLevel option. So I'm not fond of this approach for my part. Maybe we should just do nothing more than the code from @billyrennekamp , and let the users set the zoom.init.level option according to minLevel value ?

Indigo744 commented 8 years ago

There is a side effect indeed.

I agree with you to let the user decide. If he wants to initialize the zoom, there is already an option available.

Let's merge this PR :)

neveldo commented 8 years ago

Done :)