kepler.gl
kepler.gl copied to clipboard
keplergl
[Bug] Fix custom map style input
Fix for #2560 and #2458: custom map style urls aren't being loaded.
Quoting from #2560:
Looks like the issue (or one issue) is that the confirmButton in the AddMapStyle modal is always disabled. It's checking for mapStyle.inputStyle.style which is always null since the input onChange listener only sets a url and not a style property. Also addCustomMapStyleUpdater is checking for state.inputStyle.id which is also null. Setting any id from the onChange handler seems to fix the issue.
The icon is also broken. The getStyleImageIcon called from inputMapStyleUpdater is returning a Mapbox url even for non-mapbox basemaps. For instance, inputting the example url "https://basemaps.cartocdn.com/gl/positron-gl-style/style.json" gives me "https://api.mapbox.com/styles/v1/https://basemaps.cartocdn.com/gl/positron-gl-style/style.json/static/-122.3391,37.7922,9,0,0/400x300?access_token={snip}&logo=false&attribution=false". I'm not sure what the best fix for that would be.
Deploy Preview for keplergl2 ready!
| Name | Link |
|---|---|
| Latest commit | cd4d406d56761d464a657767f2f5e634ad620d4d |
| Latest deploy log | https://app.netlify.com/sites/keplergl2/deploys/6690042f2bfa3f0008de5c3f |
| Deploy Preview | https://deploy-preview-2564--keplergl2.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
The icon is also broken. The getStyleImageIcon called from inputMapStyleUpdater is returning a Mapbox url even for non-mapbox basemaps. For instance, inputting the example url "https://basemaps.cartocdn.com/gl/positron-gl-style/style.json" gives me "https://api.mapbox.com/styles/v1/https://basemaps.cartocdn.com/gl/positron-gl-style/style.json/static/-122.3391,37.7922,9,0,0/400x300?access_token={snip}&logo=false&attribution=false". I'm not sure what the best fix for that would be.
Looking at your URL it seems the style is expected to be a name not a URL (see the double https in https://api.mapbox.com/styles/v1/https://basemaps.cartocdn.com/gl/positron-gl).
So while this PR may unbreak some thing, it is probably not a complete solution.
@ilyabo @macrigiuseppe Any thoughts?
I can confirm that the main issue is still broken with just a mapbox url scheme:
https://github.com/keplergl/kepler.gl/assets/931368/23aa7a69-2186-40f8-a31a-37af3aac941f
With this fix, the mapbox:// url scheme still doesn't work. Looks like the only two places that check for a mapbox url scheme are isStyleUsingMapboxTiles (called to determine attribution in map-container.tsx) and getStyleDownloadUrl (called from getLoadMapStyleTasks)
https://github.com/keplergl/kepler.gl/assets/931368/faf578e9-c335-4c82-ac4e-c9b093d16178
Maplibre dropped the support for mapbox:// style URLs. To work around this we would probably have to load the basemap library dynamically. Maybe we can add a switch in this dialog to choose from Maplibre and Mapbox. Or revert to Mapbox. @ibgreen what do you think?
I don't think that reverting to mapbox is what we want to do. Supporting both would be an option, though that is a feature that someone needs to implement.
One option is to remove the map style support and the dialog.
How important is this feature? Are we breaking a important workflows here?
Would it be possible to simply rewrite the urls? Either in Kepler or in Maplibre? This feels like a pretty useful feature
Or at the very least include a warning message if you try to use a mapbox:// url scheme? This feels unexpected
I suspect that newer mapbox styles would not work correctly with maplibre. I have experienced issues when trying to use newer styles with mapbox@1.
I can look into the icon import. It feels odd that the importer would assume that all urls come from mapbox, but it's not clear to me that Carto, for instance, returns a unique icon for all of its styles, or that every other style provider is required to return one as well. It seems to me like this would be better if it just used a generic icon, or black square.
I do think it's fine for mapbox:// urls to be broken so long as basic style.jsons still work. What do you think?
It doesn't really make sense to continue supporting Mapbox styles (or their URLs) unless the actual Mapbox library is added as an alternative option, right?
Maybe not the url protocol but it's the most commonly used map style format. What's the alternative?
And how have they diverged? The functionality to load mapbox V1 styles works after a bugfix. I don't feel like having a preview icon is enough reason to break this workflow. What do you think?
Apologies that this is taking time, the basemap support in kepler clearly lacks a decisive owner right now. Together we all have parts of the answer, but the risk is that this thread could go on for a long time unless we decide on something. Following up in the issue.
Yeah totally agree. I don't want to bike shed this any more than necessary. I agree there needs to be a better default for the icon. If I come up a patch to make this black or a basic color or something would that be acceptable?
come up a patch to make this black or a basic color or something would that be acceptable?
Yes let's land something reasonable.
It doesn't really make sense to continue supporting Mapbox styles (or their URLs) unless the actual Mapbox library is added as an alternative option, right?
I think this is right. The reason mapbox:// was removed from maplibre was not only a matter of compatibility, but also a license compliance concern. This change in the mapbox style spec license saying:
The software and files in this repository (collectively, "Software") are licensed under the Mapbox TOS for use only with the relevant Mapbox product(s) listed at www.mapbox.com/pricing.
Looking at the www.mapbox.com/pricing etc. it seemed risky to interpret that in any other way than "mapbox styles coming from the mapbox api will be under latest version of this license, and are thus only allowed to be rendered with a mapbox renderer".
I added a temporary fix using the NO_BASEMAP_ICON for custom map styles and removing the logic for isValidStyleUrl which checked for mapbox styles. I'm sure this can be changed in the future but for now this fixes the major issues.
@birkskyum oddly tests are passing for me locally. Not sure what's happening on CI?
Weird,. Maybe the ci has different node version, or OS. What's your environment like?
I'm on Mac node v20.9.0. That test is a unit test that isn't using a Chrome instance, so I'm not sure why it would be massively different. Are you able to run it locally?
No, I see it fail locally too, with yarn test-node-debug and node 18 (it's the version used in the CI)
Wasn't aware of this PR before creating a new bug report. Just wanted to let you all know that Kepler.gl has been indispensable for our hobby project for geospatial analysis. Adding a custom map style helped in topographic migration analysis, and it's a bummer it didn't work. Would absolutely love to see the fix merged!
- https://github.com/keplergl/kepler.gl/issues/2581
