-
Notifications
You must be signed in to change notification settings - Fork 16.2k
feat(deckgl): REREVERT: add support for OpenStreetMap as our new default and ke "tile-providers" more configurable #34176
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
Conversation
…ke "tile-providers" more configurable (#33603) Co-authored-by: Maxime Beauchemin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Incomplete Warning About Configuration Override ▹ view | 🧠 Not in scope | |
| Incorrect Configuration Key for OSM Tiles ▹ view | 🧠 Not in standard | |
| Missing Default Value for Map Tiles Configuration ▹ view | 🧠 Not in scope | |
| Improper Sentence Structure ▹ view | 🧠 Not in scope | |
| Unclear Documentation Grammar ▹ view | 🧠 Not in standard | |
| Unclear Parameter Name Abbreviation ▹ view | 🧠 Incorrect | |
| Incorrect CORS Origin Format ▹ view | 🧠 Not in scope | |
| Props Mutation in Tile Layer Addition ▹ view | 🧠 Not in scope | |
| Hard-to-Parse Conditional Logic ▹ view | 🧠 Not in scope | |
| Unvalidated Tile Source URL ▹ view | 🧠 Incorrect |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/packages/superset-ui-core/src/validator/validateMapboxStylesUrl.ts | ✅ |
| superset/examples/misc_dashboard.py | ✅ |
| docs/docs/configuration/map-tiles.mdx | ✅ |
| superset/examples/long_lat.py | ✅ |
| superset-frontend/plugins/legacy-preset-chart-deckgl/src/DeckGLContainer.tsx | ✅ |
| superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils.ts | ✅ |
| superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx | ✅ |
| superset/views/base.py | ✅ |
| superset/examples/deck.py | ✅ |
| superset/config.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
|
|
||
| # Map tiles | ||
|
|
||
| Superset uses OSM and Mapbox tiles by default. OSM is free but you still need setting your MAPBOX_API_KEY if you want to use mapbox maps. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const layers = useCallback(() => { | ||
| if ( | ||
| (props.mapStyle?.startsWith(TILE_LAYER_PREFIX) || | ||
| OSM_LAYER_KEYWORDS.some(tilek => props.mapStyle?.includes(tilek))) && |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| CORS_OPTIONS: dict[Any, Any] = { | ||
| "origins": [ | ||
| "https://tile.openstreetmap.org", | ||
| "https://tile.osm.ch", | ||
| "https://your_personal_url/{z}/{x}/{y}.png", | ||
| ] | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| Map tiles can be set with `DECKGL_BASE_MAP` in your `superset_config.py` or `superset_config_docker.py` | ||
| For adding your own map tiles, you can use the following format. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| :::warning | ||
| Setting `DECKGL_BASE_MAP` overwrite default values | ||
| ::: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| "all_columns_x": "LON", | ||
| "all_columns_y": "LAT", | ||
| "mapbox_style": "mapbox://styles/mapbox/light-v9", | ||
| "mapbox_style": "https://tile.openstreetmap.org/{z}/{x}/{y}.png", |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| "d3_format": conf.get("D3_FORMAT"), | ||
| "d3_time_format": conf.get("D3_TIME_FORMAT"), | ||
| "currencies": conf.get("CURRENCIES"), | ||
| "deckgl_tiles": conf.get("DECKGL_BASE_MAP"), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| props.layers.unshift( | ||
| buildTileLayer( | ||
| (props.mapStyle ?? '').replace(TILE_LAYER_PREFIX, ''), | ||
| 'tile-layer', | ||
| ), | ||
| ); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if ( | ||
| (props.mapStyle?.startsWith(TILE_LAYER_PREFIX) || | ||
| OSM_LAYER_KEYWORDS.some(tilek => props.mapStyle?.includes(tilek))) && | ||
| props.layers.some( | ||
| l => typeof l !== 'function' && l?.id === 'tile-layer', | ||
| ) === false | ||
| ) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| export function buildTileLayer(url: string, id: string) { | ||
| interface TileLayerProps { | ||
| id: string; | ||
| data: string; | ||
| minZoom: number; | ||
| maxZoom: number; | ||
| tileSize: number; | ||
| renderSubLayers: (props: any) => (BitmapLayer | PathLayer)[]; | ||
| } | ||
|
|
||
| interface RenderSubLayerProps { | ||
| tile: { | ||
| bbox: GeoBoundingBox; | ||
| }; | ||
| data: any; | ||
| } | ||
|
|
||
| return new TileLayer({ | ||
| data: url, | ||
| id, | ||
| minZoom: 0, | ||
| maxZoom: 19, | ||
| tileSize: 256, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@mistercrunch I have somehow fixed the problem when printing deckgl map and also set deckgl library version in folder legacy-preset-chart-deckg to 9.1.3, |
|
@mistercrunch Is it possible to get access to this PR, so I can contribute for fixing ? |
Unfortunately I can't give you access to the ASF (apache) org, there's a complex process to do this and it's managed by the ASF, but you can open a new PR from your fork, happy to close this one in favor of yours. Can also try to put in your suggested fix in package.json real quick. Giving it a quick shot but don't want to block you if you want to open a new PR. |
Seems this belongs in I'll push my interpretation of your fix to see if it works ... Feel free to fork but make sure to add deck.gl libs in the plugin not the main app... |
| "@deck.gl/core": "^9.0.37", | ||
| "@deck.gl/layers": "^9.0.38", | ||
| "@deck.gl/react": "^9.1.4", | ||
| "@deck.gl/aggregation-layers": "^9.1.13", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plavacquery trying your suggested fix (upgraded to 9.1.13) while keeping the sub-packages, and adding luma.gl deps explicitely
|
Tried a few things locally and bumped into dep hell, passing the baton as I can't take this on RN. @plavacquery please open a new PR from your fork if you can. Still hoping we can push forward. |
|
Oh, this didn't work locally but seems to work on CI, the hypothesis around getting |
|
Seems to be pointing around webpack / mapbox / es6 support... |
|
I was struggling to get openstreetmaps to be default view to work with the pip superset version by changing settings but it just wouldnt work. Your github changes made getting openstreetmaps to work perfectly without much trouble. If anyone is struggling to get it to work the only thing i needed to do is: #32187 . that specific one as the config-map-tiles sql lab didnt work so well and then follow |
|
Another PR tackled this, closing |
Somehow this PR #34017 reverted a bunch of stuff from #33603 , guessing this was a badly resolved merge conflict and people missed in around review/merge