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

fitBounds with padding zooms the map really far out #4528

Closed
theashyster opened this Issue May 3, 2016 · 16 comments

Comments

Projects
None yet
10 participants
@theashyster
Contributor

theashyster commented May 3, 2016

I am not sure if this is a regression or not, but I have experienced that since the RC1 release the map fitBounds function with padding option does not work the same way as it worked before.

Some examples:
Leaflet 0.7.5 http://playground-leaflet.rhcloud.com/niv/edit?html,output
Leaflet 1.0.0-beta.2 http://playground-leaflet.rhcloud.com/jasi/edit?html,output
Leaflet 1.0.0-rc.1 http://playground-leaflet.rhcloud.com/cipe/edit?html,output

The padding is just really big and the map is zoomed far out. We have used this function with the padding on layer containing SVG polygon also and since RC1 that also started to act strange by zooming the map really far out.

I can get the same result on RC1 by setting the padding option on a point that is really small L.point(0.001, 0.001).

@IvanSanchez IvanSanchez added the bug label May 3, 2016

@IvanSanchez IvanSanchez added this to the 1.0 milestone May 3, 2016

@jieter

This comment has been minimized.

Show comment
Hide comment
@jieter

jieter May 3, 2016

Contributor

Sounds like a perfect chance for a simple unit test and some git bisecting...

Contributor

jieter commented May 3, 2016

Sounds like a perfect chance for a simple unit test and some git bisecting...

@IvanSanchez

This comment has been minimized.

Show comment
Hide comment
@IvanSanchez

IvanSanchez May 3, 2016

Member

The culprit seems to be this line:

boundsSize = this.project(se, zoom).subtract(this.project(nw, zoom)).add(padding),

That's getting the size (in pixels) of the bounds to fit, at the current zoom level, then adding the padding.

What we really want is to have the padding at the destination zoom level, not at the original zoom level.

Member

IvanSanchez commented May 3, 2016

The culprit seems to be this line:

boundsSize = this.project(se, zoom).subtract(this.project(nw, zoom)).add(padding),

That's getting the size (in pixels) of the bounds to fit, at the current zoom level, then adding the padding.

What we really want is to have the padding at the destination zoom level, not at the original zoom level.

@IvanSanchez

This comment has been minimized.

Show comment
Hide comment
@IvanSanchez

IvanSanchez May 3, 2016

Member

A possible solution would be to subtract padding from size - but that needs testing.

I think this is a rather self-contained, explained bug, so I think any newbie can try fixing this. (Hello, @yourfirstpr!).

Things to be done:

  • Let people know you're interested
  • Fork the leaflet repo and clone it locally
  • Modify src/map/Map.js around line 509 so that padding is subtracted from size (instead of being added to boundsSize)
  • Add some unit tests to spec/map/MapSpec.js, next to the tests for getBounds() - ensure that calls to getBoundsZoom() with small areas and big padding returns high levels of zoom (currently it returns low levels like 0 and 1)
  • Run npm install, npm test
  • git commit, git push
  • Create merge request
Member

IvanSanchez commented May 3, 2016

A possible solution would be to subtract padding from size - but that needs testing.

I think this is a rather self-contained, explained bug, so I think any newbie can try fixing this. (Hello, @yourfirstpr!).

Things to be done:

  • Let people know you're interested
  • Fork the leaflet repo and clone it locally
  • Modify src/map/Map.js around line 509 so that padding is subtracted from size (instead of being added to boundsSize)
  • Add some unit tests to spec/map/MapSpec.js, next to the tests for getBounds() - ensure that calls to getBoundsZoom() with small areas and big padding returns high levels of zoom (currently it returns low levels like 0 and 1)
  • Run npm install, npm test
  • git commit, git push
  • Create merge request
@varjmes

This comment has been minimized.

Show comment
Hide comment
@varjmes

varjmes May 3, 2016

👋 Thanks so much for another wonderful guide, @IvanSanchez! Will post this out to @yourfirstpr shortly :)

varjmes commented May 3, 2016

👋 Thanks so much for another wonderful guide, @IvanSanchez! Will post this out to @yourfirstpr shortly :)

@theashyster

This comment has been minimized.

Show comment
Hide comment
@theashyster

theashyster May 3, 2016

Contributor

@IvanSanchez Looks like this is related then #4172

Contributor

theashyster commented May 3, 2016

@IvanSanchez Looks like this is related then #4172

@theashyster

This comment has been minimized.

Show comment
Hide comment
@theashyster

theashyster May 3, 2016

Contributor

Tried subtract padding from size and not adding padding to boundsSize and it seems to have helped with this case.

Contributor

theashyster commented May 3, 2016

Tried subtract padding from size and not adding padding to boundsSize and it seems to have helped with this case.

@dianjin

This comment has been minimized.

Show comment
Hide comment
@dianjin

dianjin May 3, 2016

Contributor

I would love to do this! Thanks for the guide, I'll follow it and let you know my results. 😄

Contributor

dianjin commented May 3, 2016

I would love to do this! Thanks for the guide, I'll follow it and let you know my results. 😄

@hyperknot

This comment has been minimized.

Show comment
Hide comment
@hyperknot

hyperknot May 3, 2016

Collaborator

I'd just like to add to the list:

  • test it with the default zoomSnap: 1 option
  • test it with a fractional zoomSnap, like 0.25
  • test it with zoomSnap: false or 0

For development of fitBounds I believe the best is to use zoomSnap: 0, as snapping to an integer zoom level will usually add a big padding on it's own.

Collaborator

hyperknot commented May 3, 2016

I'd just like to add to the list:

  • test it with the default zoomSnap: 1 option
  • test it with a fractional zoomSnap, like 0.25
  • test it with zoomSnap: false or 0

For development of fitBounds I believe the best is to use zoomSnap: 0, as snapping to an integer zoom level will usually add a big padding on it's own.

@dianjin

This comment has been minimized.

Show comment
Hide comment
@dianjin

dianjin May 3, 2016

Contributor

Here's what I have so far: #4532

Please let me know if I did everything correctly!

Contributor

dianjin commented May 3, 2016

Here's what I have so far: #4532

Please let me know if I did everything correctly!

@hyperknot

This comment has been minimized.

Show comment
Hide comment
@hyperknot

hyperknot May 3, 2016

Collaborator

@dianjin code looks good for me, wrote a comment about the test.

Feel free to tick the completed checkboxes on this page.

Collaborator

hyperknot commented May 3, 2016

@dianjin code looks good for me, wrote a comment about the test.

Feel free to tick the completed checkboxes on this page.

@IvanSanchez

This comment has been minimized.

Show comment
Hide comment
@IvanSanchez

IvanSanchez May 9, 2016

Member

Closed via #4532

Member

IvanSanchez commented May 9, 2016

Closed via #4532

@theashyster

This comment has been minimized.

Show comment
Hide comment
@theashyster

theashyster May 9, 2016

Contributor

Thanks again for fixing the issue 👍

Contributor

theashyster commented May 9, 2016

Thanks again for fixing the issue 👍

@eldamsheety

This comment has been minimized.

Show comment
Hide comment
@eldamsheety

eldamsheety Nov 15, 2017

how could I start joining an open source here?

how could I start joining an open source here?

@aalsiuser

This comment has been minimized.

Show comment
Hide comment
@aalsiuser

aalsiuser Feb 10, 2018

As Iam new to open source community so how can i help you in contributing over here?

As Iam new to open source community so how can i help you in contributing over here?

@qwertypool

This comment has been minimized.

Show comment
Hide comment
@qwertypool

qwertypool Feb 11, 2018

hello, how may i join and start contributing to this open source ??

hello, how may i join and start contributing to this open source ??

@ghybs

This comment has been minimized.

Show comment
Hide comment
@ghybs

ghybs Feb 12, 2018

Collaborator

Hi @eldamsheety, @aalsiuser and @qwertypool,

Thank you for your interest in Leaflet!
Sorry for not getting back to you earlier.

There are many ways of contributing to this project, and to Open Source projects in general.

This thread is already closed, since the bug it describes has already been fixed in PR #4532.
Avoid commenting on closed threads, as you may remain unnoticed, unless what you have to say is absolutely relevant to the thread (e.g. when the bug is actually not fixed). And even in that case, it is probably better opening a new issue and mentioning the closed thread.

As for contributing, you could have a look at threads with label "help wanted" and/or "good first issue".

Keep up the good work and feel free to ask! 😄

Collaborator

ghybs commented Feb 12, 2018

Hi @eldamsheety, @aalsiuser and @qwertypool,

Thank you for your interest in Leaflet!
Sorry for not getting back to you earlier.

There are many ways of contributing to this project, and to Open Source projects in general.

This thread is already closed, since the bug it describes has already been fixed in PR #4532.
Avoid commenting on closed threads, as you may remain unnoticed, unless what you have to say is absolutely relevant to the thread (e.g. when the bug is actually not fixed). And even in that case, it is probably better opening a new issue and mentioning the closed thread.

As for contributing, you could have a look at threads with label "help wanted" and/or "good first issue".

Keep up the good work and feel free to ask! 😄

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