Add zoom range restriction to keepCurrentZoomLevel

[barryhunter]

Fixes #246

[domoritz]

Thank you for the PR. I have two small comments but would be happy to merge once you addressed them.

[domoritz]

Please correct the grammar of this sentence. Also, end with a ..

[barryhunter]

Not sure what the grammer issue is, but have just spotted the rage=>range typo. I'll try fixing this - I think as a new PR.

[domoritz]

Something like this (it wasn't clear what the which clause refers to):

Suggested change

	| `keepCurrentZoomLevel` | `boolean` or `Array` | Only pan when setting the view. Set to True, or a zoom range like [13,18] which will only zoom the map when outside the rage. Allows keeping the current view at these scales | `false` |
	| `keepCurrentZoomLevel` | `boolean` or `Array` | Only pan when setting the view. Setting a zoom range such as `[13,18]` will only zoom the map if the current zoom level is outside the specified range. | `false` |

[barryhunter]

just saw reply. I've updated the wording, try to keep clearer. It only zooms when outside the range. It keeps the zoom when within the range! ie keep zoom (if True, or within set range) barryhunter@1eaee57
not clear if that commit, is feeding back to pull request. if not will make a new one, after resolving other comment)

[domoritz]

Can you extract this into a variable zoomOutsideRange and then write if (this.options.keepCurrentZoomLevel || zoomOutsideRange)?

[barryhunter]

Not sure what mean, make something like

  var zoomOutsideRange = typeof this.options.keepCurrentZoomLevel == "object" && this._map.getZoom() <= this.options.keepCurrentZoomLevel[1] && this._map.getZoom() >= this.options.keepCurrentZoomLevel[0];

if (this.options.keepCurrentZoomLevel) would still be true in case of Array, so the if(..) still complicated.

although maybe could do if (this.options.keepCurrentZoomLevel === true || zoomOutsideRange)? (not used === in javascript!)

---

Or to have new this.options.zoomOutsideRange to store the range?

  var zoomOutsideRange = typeof this.options.zoomOutsideRange == "object" && this._map.getZoom() <= this.options.zoomOutsideRange[1] && this._map.getZoom() >= this.options.zoomOutsideRange[0];

Then could just do... keepCurrentZoomLevel is back to just true/false

if (this.options.keepCurrentZoomLevel || zoomOutsideRange )

[domoritz]

Hmm, thinking about this a bit more, I am not sure we want to merge this yet. When I see an array for keepCurrentZoomLevel, I'd think that this is providing the range that the zoom should be in (i.e. the target zoom level). I feel that filtering when to zoom is something that is very specific and should be done outside.

I'd say let's add an event that gets triggered so that you can implement your own logic for zooming in, okay?

I'm going to close this PR for now.

I hope you understand that I want to keep only core functionality as otherwise this library will get too complex to maintain.

[barryhunter]

ok. sure

but I'd think that this is providing the range that the zoom should be in (i.e. the target zoom level).

that is what is doing.

If define as [13,18] then, if outside that range, it WILL zoom to that range. Its only if its already within 13->18 that it doesnt zoom. ie keep current zoom, if already within 13->18.

[domoritz]

Not quite. What I thought a range would be a is a range to clamp to. For example, if the user defines [13,18], then the zoom level would be between 13 and 18 but we might still zoom. But you are right that this isn't quite compatible with keepCurrentZoomLevel as a name, which implies that the range has something to do with keeping the zoom level. I think now I understand it better and updated the wording a bit. Now I also see that this is quite useful and so let's merge it.

Sorry, I needed a moment to understand this proposal fully.

[domoritz]

Could you do these two things to finish the PR?

• Have a look at Add zoom range restriction to keepCurrentZoomLevel #247 (comment) and see whether you want to apply the suggestion.
• Apply your refactoring to extract zoomOutsideRange as you discussed in zoomOutsideRange.

Thanks!

[barryhunter]

So are thinking to have as a new separate 'option', as opposed to overloading keepCurrentZoomLevel.
ie add 'options.zoomOutsideRange'.

Or is it just reflectoring the complicated if(..) clause that concerned about?

[domoritz]

I think keeping it part of keepCurrentZoomLevel is fine (no separate option). I am concerned about the docs (they need to be crystal clear so that others don't run into my confusions ;-)) and the complicated if clause.