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

docs(mapbox) discoverability for integration examples #7734

Closed
wants to merge 1 commit into from

Conversation

chrisgervang
Copy link
Collaborator

Change List

  • update standalone version
  • use maplibre
  • example links

@@ -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)
Copy link
Collaborator Author

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?

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

@coveralls
Copy link

Coverage Status

Coverage: 90.056% (-0.0002%) from 90.056% when pulling de461ba on chr/mapbox-docs into 4493f2f on master.

<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' />
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

@chrisgervang
Copy link
Collaborator Author

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).

@chrisgervang chrisgervang deleted the chr/mapbox-docs branch March 7, 2024 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants