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

Support float zooms in bounds() function #19

Merged
merged 5 commits into from May 29, 2018

Conversation

@timiyay
Copy link
Contributor

commented May 25, 2018

This PR adds support for float zooms (i.e 16.5) in the bounds() function, satisfying issue #5.

The real work was done upstream in https://github.com/mapbox/sphericalmercator, and this PR only required a small amount of change:

• adding a test for float-based zoom levels
• updating node-sphericalmercator from v1.0.2 to v1.1.0 (latest at time of writing)

timiyay added some commits May 24, 2018

Strips trailing whitespace from package.json
This formatting change was generated automatically when running
`npm install` with npm v6.1.0.
Upgrades node-sphericalmercator to v1.1.0
This version now supports float-based zoom levels.
"dependencies": {
"@mapbox/sphericalmercator": "~1.0.2"
},
"@mapbox/sphericalmercator": "~1.1.0"

This comment has been minimized.

Copy link
@timiyay

timiyay May 25, 2018

Author Contributor

This is the meat of the PR - updating a dependency. Easy peasy!

Unfortunately, the diff is a bit noisy for the package.json. I'm developing on npm 6, and it insisted on removing the trailing whitespace from every line in the file. Nonetheless, I left it in, as it isn't a difficult change to follow.

@timiyay

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2018

@mapsam I've had a crack at adding float zoom support to bounds(). It's quite a small change. Whenever you have time and inclination, I'd love a review!

t.end();
});

test('bounds for float zooms', function (t) {

This comment has been minimized.

Copy link
@timiyay

timiyay May 25, 2018

Author Contributor

If you're wondering where the magic values came from, in these test expectations...

I didn't have the necessary background in the node-sphericalmercator math to create my expectations with math, so I fell back to good old GIS.

My original use case for this module was creating GeoJSON bounding boxes that bound Mapbox Static Map API images. So, I used this as my reality check. I'd use the API to request an image, use bounds() with my API request params (centre, zoom, px width, px height) to determine its bbox, then use QGIS to do a visual check.

To that end, it's slightly imprecise, but matches my use case.

This comment has been minimized.

Copy link
@mapsam

mapsam May 29, 2018

Member

I think this is fine. One thing to do would be add a test for z 16 and z 16.5 and assert that the values are less/greater than each other, essentially asserting they output different values.

No need to do this now, though!

@mapsam

mapsam approved these changes May 29, 2018

Copy link
Member

left a comment

Thanks a lot, @timiyay! Looks great - I'll release this as version 0.3.0
👍

t.end();
});

test('bounds for float zooms', function (t) {

This comment has been minimized.

Copy link
@mapsam

mapsam May 29, 2018

Member

I think this is fine. One thing to do would be add a test for z 16 and z 16.5 and assert that the values are less/greater than each other, essentially asserting they output different values.

No need to do this now, though!

@mapsam mapsam merged commit d62873a into mapbox:master May 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@timiyay timiyay deleted the timiyay:float-zooms-bounds branch May 29, 2018

@MichielDeMey MichielDeMey referenced this pull request Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.