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 worldmap.js to allow "flying" to new bounds #225

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

rmwiseman
Copy link
Contributor

When the bounds command is used, give the option to "fly" (a Leaflet term meaning to animate the change of bounds: zoom out, pan, zoom in) to the new bounds rather than simply repositioning the map. This is done very simply by providing a "fly" flag that, with a truthy value, calls map.flyToBounds instead of map.fitBounds.

There are 10 other places where map.fitBounds is used, but for my purposes, I only want to "fly" to the new bounds when I explicitly set the bounds through this command. So perhaps these changes could/should be used in other scenarios too.

When the bounds command is used, give the option to "fly" (a Leaflet term meaning to animate the change of bounds: zoom out, pan, zoom in) to the new bounds rather than simply repositioning the map.  This is done very simply by providing a "fly" flag that, with a truthy value, calls map.flyToBounds instead of map.fitBounds.

There are 10 other places where map.fitBounds is used, but for my purposes, I only want to "fly" to the new bounds when I explicitly set the bounds through this command.  So perhaps these changes could/should be used in other scenarios too.
@dceejay
Copy link
Owner

dceejay commented Apr 4, 2023

Interesting thought. I think for the wider usage - many of the others are triggered by a property fit being present - so maybe add fly as an option in those cases (rather than require both fit and fly).

@rmwiseman
Copy link
Contributor Author

I agree, that makes sense. Is this something I should change in my patch...? (It looks like I can still edit it.)

One possible issue I noticed just now is that one fitBounds call is immediately followed by a panTo call (which, now I think about it, seems like a strange sequence).

Testing might be an issue for me as most instances of fit are on features I don't use, but presumably there are automated tests you run before each release?

I've changed almost every instance of fitBounds so that, if the "fly" property is true, then it will use flyToBounds instead.

I didn't change the search, where it does fitBounds and then panTo, as it wasn't clear how/whether "fly" could be provided there.

I also fixed a few bugs where it was checking for "fit" being a property, but then didn't check its value!  So you could provide fit:false and it would still be treated as fit:true!
@rmwiseman
Copy link
Contributor Author

rmwiseman commented Apr 5, 2023

I've changed my patch and it now seems to be checking the changes for this pull request, so perhaps that answers my question!

As mentioned in the comment on the change I just made, it looks like a minor bug existed in a few places: it was checking whether fit was a property but then didn't check its value, so if someone used fit: false it would still fit it. I've changed those to ensure fit is true. I also changed my original patch so that fly needs to be true not just truthy, as that aligns better with the existing code.

@dceejay dceejay merged commit 7ee1204 into dceejay:master Apr 11, 2023
@dceejay
Copy link
Owner

dceejay commented Apr 11, 2023

Excellent - thanks.
Released as v2.37.0

@rmwiseman rmwiseman deleted the patch-2 branch April 17, 2023 09:47
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