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

Map.panInsideBounds: lookahead and center view #1946

Closed
wants to merge 2 commits into from

Conversation

kapouer
Copy link
Contributor

@kapouer kapouer commented Aug 7, 2013

This avoids loops and provides a sensible fix
for #1908.

@mourner
Copy link
Member

mourner commented Aug 7, 2013

Please fix the build errors.

@mourner
Copy link
Member

mourner commented Aug 7, 2013

Thanks! Looks like a good way to approach this, will probably be merged.

@mourner
Copy link
Member

mourner commented Aug 8, 2013

It would also be great to add some tests to cover the maxBounds behavior along with these changes so that we don't accidentally regress in future.

@kapouer
Copy link
Contributor Author

kapouer commented Aug 9, 2013

May i mention that i think enforcing minzoom is not the right behavior ? Giving a way to calculate minzoom easily is nice, though. I won't bother patching this because doing map._minBoundsZoom = null is enough for me.

@kapouer
Copy link
Contributor Author

kapouer commented Aug 25, 2013

Is there something wrong with that pull request ?

@mourner
Copy link
Member

mourner commented Aug 25, 2013

Sorry, no, it's just that I'm on vacation atm, coming back on Tuesday. :)

@kapouer
Copy link
Contributor Author

kapouer commented Sep 13, 2013

Antibump : i'll add another commit on top of this, to disable automatic _boundsMinZoom which gets in the way of this new panInsideBounds.

@mourner
Copy link
Member

mourner commented Oct 10, 2013

Ooops, somehow I missed notifications of this being worked on. Looks pretty good! Will do some testing and then merge. Thanks Jérémy!

@mourner
Copy link
Member

mourner commented Oct 10, 2013

About zoom limiting... If we do remove it and then rely on users setting minZoom explicitly in case they need it, it won't work exactly the same, as the minZoom adjusts depending on the size of the map which can be fluid. Should we make this behavior an option instead of removing it?

@kapouer
Copy link
Contributor Author

kapouer commented Oct 10, 2013

I don't think it's that useful an option, but am ok with changing the PR to make it one.

@mourner
Copy link
Member

mourner commented Oct 10, 2013

@danzel @tmcw @jfirebaugh should we add an option, or ditch the dynamic minZoom, what do guys you think?

@danzel
Copy link
Member

danzel commented Oct 10, 2013

Did anyone request it in the first place or did we add it because we thought it'd be cool?
I'm leaning towards remove it and see if anyone complains.

@mourner
Copy link
Member

mourner commented Oct 10, 2013

I think I added it because I thought it made sense, but I could be wrong.

@mourner
Copy link
Member

mourner commented Oct 11, 2013

OK, tested the interaction on the debug/map/max-bounds.html page, and it doesn't seem to work very well... Jump after each zoom beyond the "min" one looks weird (maybe it would be much smoother if it determined the right zoom origin and zoomed into correct place from the start), and also the behavior gets confusing when the screen fits the bounds on one dimension but not on the other - in the example above, you can't look at the bottom of UK on certain zoom levels at all, because it jumps back to the center.

@kapouer
Copy link
Contributor Author

kapouer commented Oct 11, 2013

I've tested with only square-ish bounds... i'll push fixes asap.

Deltas are calculated on x, y pixel coordinates separately.
Options are propagated from setMaxBounds to panBy.
No panBy loops. Fixes Leaflet#1908.
@kapouer
Copy link
Contributor Author

kapouer commented Oct 14, 2013

It still jumps, but it does calculate it on each direction independently.
Rewrote into one commit + added tests.

About the "jumps": panBy is useful to make the user realize it's out of bounds.
So i'm in favor of keeping it that way.
Patching the code calculating the zoom origin can be done later, it's independent.

@mourner
Copy link
Member

mourner commented Nov 7, 2013

Going to wrap up with 0.7 during next week and this will get merged, maybe with some tweaks, fixing a big bunch of maxBounds-related issues. Thanks a lot for working on this @kapouer!

@mourner
Copy link
Member

mourner commented Nov 14, 2013

Made a new pull based on your work, with proper zoom anchoring and code cleanup.

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

Successfully merging this pull request may close these issues.

None yet

3 participants