Tilesets native zoom #792
Tilesets native zoom #792
Conversation
Plus some small cosmetic changes
folium/raster_layers.py
Outdated
| - "Cloudmade" (Must pass API key) | ||
| - "Mapbox" (Must pass API key) | ||
| - "CartoDB" (positron and dark_matter) | ||
|
|
||
|
|
There was a problem hiding this comment.
W293 blank line contains whitespace
I don't think we should hardcode known values. The grey tiles are "expected" in the slippy mapping world when you go over the max zoom. People who want a "nice" zoom should do that explicitly.
We can consolidate the tile templates, this is a good idea, but I don't think we should provide the maz values.
Dismissed. Having the option to add the value manually is good enough IMO. |
|
Thanks for the quick feedback! I can agree with sticking to manually adding a value. The whole storing and verifying of the values is a bit messy, though interesting to investigate. I'll revert the changes past the keyword part. |
folium/raster_layers.py
Outdated
| 'attribution': attr, 'subdomains': subdomains, | ||
| 'detectRetina': detect_retina} | ||
| if max_native_zoom: | ||
| options['maxNativeZoom'] = max_native_zoom |
There was a problem hiding this comment.
This is fine but I am 99% sure you can just pass the option and, if None, the json.dumps will convert to null and everything will work as expected on the JS side. Then we'll have 1 new line in the options instead of the if clause.
There was a problem hiding this comment.
Unfortunately the map won't load with "maxNativeZoom": null. I understand you don't want the extra if clause. I solved it by using 'maxNativeZoom': max_native_zoom or max_zoom.
There was a problem hiding this comment.
Unfortunately the map won't load with
"maxNativeZoom": null.
Ah! I did not test that. Thanks!
I understand you don't want the extra if clause.
Actually the if-clause was OK in that case.
I solved it by using
'maxNativeZoom': max_native_zoom or max_zoom.
That is fine too. Thanks!
folium/raster_layers.py
Outdated
| } | ||
| options = {'minZoom': min_zoom, 'maxZoom': max_zoom, 'noWrap': no_wrap, | ||
| 'attribution': attr, 'subdomains': subdomains, | ||
| 'detectRetina': detect_retina} |
There was a problem hiding this comment.
The 1 option per line is by design, for readability. Do you mind reverting that change?
|
Thanks for the review @ocefpaf. I corrected the two things you pointed out. |
|
Thanks @Conengmo! The failure is b/c of our flaky tests that compare templates and results. We need to add the extra option there. But you did a lot already and I can solve this while reviewing the other PRs. Thanks! |
I worked a bit on adding a parameter for the maximum native zoom levels of a tileset, based on #789.
The simplest implementation is just accepting the keyword and adding it to the options of a
TileLayer. Since we don't know a reasonable default value, it shouldn't be added if it'sNone. I think up to here this is an uncontroversial addition.Then I worked on finding and adding the maximum native zoom level of the tilesets mentioned in the
TileLayerdocstring. If these are used automatically we prevent users from encountering grayed out tiles.For most of these tilesets you can check the maximum native zoom level automatically. So I wrote a test that verifies the stored maximum native zoom levels. The tileservers will return a HTTP 403 or 404 when requesting a tile with an unsupported zoom level. Except for the Stamen ones, I didn't find any logic there. It will return tiles for zoom levels up to 30, but will randomly return a HTTP 503 for some reason.
I'm not sure if these values should be hard-coded in
raster_layers.py. But the tiles templates also didn't seem to fit right. What I was thinking now was to make the tiles folder into a single JSON file, where each tileset is an entry with an url template, attribution and maximum native zoom level.I'm also not sure about the test. If the test fails because a tileserver changed, it may confuse a contributor who doesn't know about it.