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

feat: support Mapbox satellite styles #475

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

jessicamcinchak
Copy link
Member

@jessicamcinchak jessicamcinchak commented Aug 15, 2024

Adds new props:

  • applySatelliteStyle
  • mapboxAccessToken (needs scope style:read)

Screenshot from 2024-08-16 01-20-21

A few notes:

  • I'm not initially introducing this as an interactive toggle in the control button group, I think it's best for our free-tier prototyping to limit map renders and not allow satellite imagery to be toggled on out of curiosity across all our map implementations just yet - but lets listen for feedback around this
  • Unlike Ordnance Survey, Mapbox allows URL restrictions on their access tokens which means we should be able to move forward without a Mapbox proxy endpoint for now 👍 (Try this as example).
    • Our allow-list is simple enough for most PlanX cases, but we'll want to further test subdomain behavior & decide if/how to handle council-specific subdomains
  • I have access to the OSL Mapbox account where I set this up, but I think we want to move it to the PlanX one?

Copy link

netlify bot commented Aug 15, 2024

Deploy Preview for oslmap ready!

Name Link
🔨 Latest commit 2a8c5d6
🔍 Latest deploy log https://app.netlify.com/sites/oslmap/deploys/66be8e0f9cdd03000865c7c3
😎 Deploy Preview https://deploy-preview-475--oslmap.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jessicamcinchak jessicamcinchak requested a review from a team August 16, 2024 07:03
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a 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;
Copy link
Contributor

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?

Copy link
Member Author

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.

@jessicamcinchak
Copy link
Member Author

@DafyddLlyr our existing single key includes http://localhost:{our ports} as one-of-many-domains in the URL Restriction rules (same as BuildX config) - does that seem fine to start or you think we want fully separate keys?

@DafyddLlyr
Copy link
Contributor

I was going from this comment in the shared link above from the Mapbox docs -

Note that requests from localhost with a restricted token will be blocked unless localhost is specifically added to a token's allowed list. To develop locally, we recommend creating a separate token with more permissive URL restrictions.

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

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?

@jessicamcinchak
Copy link
Member Author

@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 👍

@jessicamcinchak jessicamcinchak merged commit deb19ca into main Aug 16, 2024
5 checks passed
@jessicamcinchak jessicamcinchak deleted the jess/support-mapbox-satellite-style branch August 16, 2024 08:25
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.

2 participants