feat: support Mapbox satellite styles#475
Conversation
✅ Deploy Preview for oslmap ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
The URL restrictions in MapBox look like a great solution 🥇 Looks like we'll need a separate dev key for localhost?
Agree that we don't want this to be a toggle always available, but we may find we need / want a toggle for the works to trees feature. So a toggle-able toggle? 😅 Let's get some feedback to decide the approach here first though 👍
There's a comment about the more general map API, which is more a follow on thought to #473 than a particular point on this PR which needs to be addressed immediately.
| osProxyEndpoint = ""; | ||
|
|
||
| @property({ type: Boolean }) | ||
| applySatelliteStyle = false; |
There was a problem hiding this comment.
We might want to think more widely about the API for this component. I think that a consumer may expect a "basemap" or "layer" prop where they can select OS Vector, OS Raster, Mapbox Satellite, or OSM (the fallback).
Right now you can pass multiple contradictory props (or incomplete props - missing keys for example) in and it's just our internal logic dictating the precedence of these internally. I think it would be better to expose this to consumers up front.
Likewise, some of the styling/drawing props could get joined up as an options object?
Worth some discussion / prototyping - seems a worthwhile consideration before v1?
There was a problem hiding this comment.
Yep fully agree - was writing the README section and remembering how round-about this is currently because of staggered implementations. Now that we have a much clearer list of supported layers, agree there's good room for reducing complexity & offering a single enum prop ✔️ I'll pick this one up shortly separately
Will maybe eventually try to start a similar v1 Github Project here for capturing all our goals - also very interested in how to get to smaller / clearer API before then and there's lots of areas for improvement.
|
@DafyddLlyr our existing single key includes |
|
I was going from this comment in the shared link above from the Mapbox docs -
Currently, somebody could take our token from PlanX / the map docs and use it to make unlimited requests locally. It does seem wiser to have a separate one from day one for local dev. I could even see this being as straightforward of keeping the local dev one in the local map repo, but using staging token (without localhost access) on planx-new This would mean no required changes to planx-new secrets (local and staging pretty much need to match), and if we want to see Mapbox satellite layers when developing we either need to copy/paste or work in the map repo? |
|
@DafyddLlyr thanks for clarifying - that makes sense and doesn't require prop changes here, so non blocking to merging which I was really double-checking. Happy to set that up and adjust account settings & env vars accordingly once reading into planx-new too 👍 |
Adds new props:
applySatelliteStylemapboxAccessToken(needs scopestyle:read)A few notes: