-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
docs(mapbox) discoverability for integration examples #7734
Conversation
@@ -30,6 +31,17 @@ npm install @deck.gl/mapbox | |||
import {MapboxOverlay} from '@deck.gl/mapbox'; | |||
``` | |||
|
|||
## Examples | |||
|
|||
[test/apps/mapbox-integration](https://github.com/visgl/deck.gl/tree/8.9-release/test/apps/mapbox-integration) |
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.
Link to master or release?
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.
We should not link to test/apps from docs. These are not maintained with the mindset that they will be used by external users. If you think our existing examples are insufficient you can add more to examples/get-started
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.
Ok, that makes sense. I'd like to maintain two public examples for the mapbox integration using interleaved MapboxOverlay with maplibre in react and pure-js since it's a common usage when someone reaches for @deck.gl/mapbox.
<script src='https://api.tiles.mapbox.com/mapbox-gl-js/v1.10.0/mapbox-gl.js'></script> | ||
<script src="https://unpkg.com/deck.gl@^8.9.0/dist.min.js"></script> | ||
<script src='https://unpkg.com/maplibre-gl@latest/dist/maplibre-gl.js'></script> | ||
<link href='https://unpkg.com/maplibre-gl@latest/dist/maplibre-gl.css' rel='stylesheet' /> |
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.
I'm not sure if it's a good idea to modify the mapbox docs for maplibre (particularly keeping it in the "mapbox" folder, but then using maplibre).
While I prefer the FOSS nature of maplibre, the deck.gl integration is still called "mapbox" and probably focused on that.
mapbox and maplibre are currently drifting apart (already):
- mapbox already has a WebGL2 context
- mapbox has a different terrain implementation from maplibre
- mapbox has non-Mercator projections
- mapbox is getting a different custom-layer API for globe projection
There might be some divergence in MapboxOverlay
in the future, too, and it might eventually only support one of the projects (with MaplibreOverlay
as additional wrapper?).
While many of the integrations should work similarly, I think it would be a good idea to explain usage of both (maplibre / mapbox) and to list what features are supported in each base-map, and which aren't.
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.
You bring up some good points to consider. We're currently focused on supporting both libraries with the same integration module since they're effectively drop-in replacements for most use cases.
Doing a hard split would add maintenance overhead to consider as well, since there are many things to test and most of the code would remain the same between the modules.
I agree showing both library options is probably more useful at this stage, as well as tracking the differences and limitations common to both, and unique to each.
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.
Doing a hard split would add maintenance overhead
Yes, it's still too early to split and guess what kind of code should or could be factored out.
For now, I think there should just be a note that "Maplibre provides the Mapbox 1.x API and can be used with the MapboxOverlay with the following limitations / additional features:"
If MapLibre is used directly in the samples, there should probably be a comment that it (currently) provides the mapbox API so users see that this is in fact a mapbox example.
I'm going to close this one. A lot of the feedback has been incorporated into v9, namely we've added separate examples for mapbox and maplibre (more in #8541) that cover different environments (pure js, react, scripting) and rendering modes (overlaid and interleaved). |
Change List