-
Notifications
You must be signed in to change notification settings - Fork 0
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -191,6 +192,12 @@ export class MyMap extends LitElement { | |||
@property({ type: String }) | |||
osProxyEndpoint = ""; | |||
|
|||
@property({ type: Boolean }) | |||
applySatelliteStyle = false; |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
applySatelliteStyle
mapboxAccessToken
(needs scopestyle:read
)A few notes: