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

.getBounds().pad() not working with negative values #5741

Closed
Shooshte opened this issue Aug 30, 2017 · 6 comments
Closed

.getBounds().pad() not working with negative values #5741

Shooshte opened this issue Aug 30, 2017 · 6 comments
Labels
docs Improvements or additions to documentation good first issue Good for newcomers help wanted

Comments

@Shooshte
Copy link

This referes to a 2 year old issue found here:
#2673

The problem:
When passing negative values into the .pad() function, the result is not smaller than the current map container (-10 and 10 produce results that differ only slightly in size, but are both bigger than the map)

Steps to reproduce:
Open the leaflet homepage map

Enter:

L.rectangle(map.getBounds().pad(-10)).addTo(map)
L.rectangle(map.getBounds().pad(10)).addTo(map)

into the console

Zoom out, see two rectangles bigger than the actual map at the time of drawing.

@CalvinWilliams1012
Copy link
Contributor

CalvinWilliams1012 commented Aug 31, 2017

I beleive that should fix the issue. However it may give a new issue as it does not check if the value is <-100 or >100.

Edit: The pull didn't pass the Travis CI because the expected values were set with the "broken" version

@danzel
Copy link
Member

danzel commented Aug 31, 2017

I think the issue here is your understanding of what the function does.

The parameter bufferRatio is a percentage, where 1 = 100%.
So 10 means, pad 1000% bigger, and -10 means -1000% bigger (which would be slightly smaller than 1000% I think).
To make it smaller you need to use a negative number that is between 0 and -0.5.
Padding by 0 would not change the size, padding by -0.5 would make it size 0.
(I think? Maybe 0.33333 would make it size 0)
This doesn't really make a lot of sense though, haha :). I don't think .pad() is designed for making a LatLngBounds smaller.
Maybe we could have another function that takes a percent, and resizes the box to that percent of the original size?

@CalvinWilliams1012
Copy link
Contributor

@danzel Ya you're right I tested it on the leaflet main page with rectangles as you can see in my mspaint pic
image

You can close/deny my pull request as I see what the function is for now. I am unsure of if we should add a new function for percentResize but I would be willing to add it :)

@IvanSanchez
Copy link
Member

I don't think this is a bug, it's just a misunderstanding of what the method does, and the fact that we are using the word "percentage" in the docstrings rather than "ratio".

Some clarification in the docstrings for that method (e.g. Returns bigger bounds created by extending the current bounds by a given ratio in each direction. For example, a ratio of 0.5 extends the bounds by 50%. Negative values yield smaller bounds instead. or something like that) would prevent this confusion in the future.

@CalvinWilliams1012, maybe you would like to make such a change in the documentation?

@CalvinWilliams1012
Copy link
Contributor

@IvanSanchez I am not sure how this projects doc's work yet I hope that comment is fine on multiple lines if not I can put it on one line, even though that makes it look gross :)

@IvanSanchez
Copy link
Member

@CalvinWilliams1012 I think #5748 looks good enough. You can always run jake docs to build the documentation and check for yourself 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation good first issue Good for newcomers help wanted
Projects
None yet
Development

No branches or pull requests

5 participants