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

Add zoomOffset option to fitBounds #6100

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sds-dubois
Copy link

Add an option to offset the zoom level after fitBounds - I feel like it's common use case to want to fit the bounds and then zoom out.

I've had some trouble calling map.zoomOut() right after fitBounds, even when disabling the animation (eg, tiles not loaded, marker positions are wrong...) and thought it would just be better to add this as an option to fitBounds.

Let me know what you think

@ghybs
Copy link
Collaborator

ghybs commented Mar 22, 2018

Hi,

Thank you for submitting your first PR here!

If I understand correctly, you would like your map to fitBounds, but with 1 zoom level lower than the current computation result?

I can understand that you would like an extra option in Leaflet core to fit your use case, but it seems to me that you could already achieve something similar with the current API, using map.getBoundsZoom(bounds):

// Compute the appropriate zoom level, without having the map actually change its view.
var fitZoom = map.getBoundsZoom(bounds);

map.setView(bounds.getCenter(), fitZoom - 1);

Example: https://plnkr.co/edit/W3dJjQBeP2RFLRTQLlRJ?p=preview

@sds-dubois
Copy link
Author

If I understand correctly, you would like your map to fitBounds, but with 1 zoom level lower than the current computation result?

Exactly, although it could be anything and not just 1 zoom level lower.

Thanks for pointing how to achieve the same result with the current API - but my point with this PR was more to suggest adding this option, just like there's the maxZoom option. I thought it could be useful to have it in fitBounds since it would also work with arrays of latlongs, padding, maximum zoom etc

But feel free to close it if you don't think that's a common enough use case.

@ghybs
Copy link
Collaborator

ghybs commented Jun 26, 2018

Hi,

Sorry for not getting back to you earlier.
I will merge this if you can add a test for this new feature.

@sds-dubois
Copy link
Author

Hi @ghybs, I just added 2 tests:

  • the first one is a simple test to verify the behavior of zoomOffset (cf the test above "Snaps zoom level to integer by default" which demonstrates zoom would be 2 otherwise)
  • a second one to test that zoomOffset does not override maxZoom if set

Btw there's no option minZoom, so let me know if you think it would be useful and I can just add it along with some tests.

@johnd0e
Copy link
Collaborator

johnd0e commented May 2, 2020

I feel like it's common use case to want to fit the bounds and then zoom out.

I'm not sure that it is really common case.
And if you really need this - why not use pad method?

I've had some trouble calling map.zoomOut() right after fitBounds, even when disabling the animation (eg, tiles not loaded, marker positions are wrong...) and thought it would just be better to add this as an option to fitBounds.

For me such option would look a bit weird. May be better address such issue in more direct way (I mean fix zoomOut method).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants