feat: clip viewport based on geojson extent#363
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.
lots of linter changes here, but this is the notable change to demo the clipped viewport !
is NOT translated to a proper Pitsby example yet, but can pick that up in a future PR.
| [-10.76418, 49.528423, 1.9134116, 61.331151], | ||
| "EPSG:4326", | ||
| "EPSG:3857", | ||
| ), |
There was a problem hiding this comment.
again, lots of linter changes here, but this is the juicy bit !
| geometry: { | ||
| coordinates: [], | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Lit default prop values for optional props can feel a bit awkward, espcially for complex types like geojson object. Might be worth revisiting this in a future PR to touch all the geojson props at once 🔖
|
Netlify deploys are passing on retry fine but not updating Github CI here - not positive what's going on there yet https://app.netlify.com/sites/oslmap/deploys/64d897e517608f7a1ff157ac |
DafyddLlyr
left a comment
DafyddLlyr
left a comment
There was a problem hiding this comment.
Pulled the branch locally, installed dependencies and then updated lockfile and all worked as expected with CI.
PR is great - two small comments to discuss but I suspect straight GeoJSON is the right fit here.
Really appreciate the helpful comments and having the list of follow up tasks laid out so clearly - super helpful. I'll start picking them up so we can get this on a Pizza this sprint.
| @property({ type: Boolean }) | ||
| showPrint = false; | ||
|
|
||
| @property({ type: Object }) |
There was a problem hiding this comment.
A simpler API might be to accept an extent e.g. [minx, miny, maxx, maxy]. We could then default to the UK extent here also.
It does put the onus on the consumer of this webcomponent to generate one however which may not suit BOPS as well as it suits us?
| new GeoJSON().readFeature(this.clipGeojsonData, { | ||
| featureProjection: "EPSG:3857", | ||
| }); | ||
| const clipExtent = clipFeature && clipFeature.getGeometry()?.getExtent(); |
There was a problem hiding this comment.
I suspect that we'll need to buffer this somewhat to get the functionality we want - the question is who's responsibility is it?
If a consumer adds a "clip" geojson but the map then shows more than they're clip that's quite odd / bad behaviour. We could add a buffer / scale value as an additional prop, or just leave as is and leave the responsibility to consumers.
I think I'd probably also go with not scaling / buffering here 👍 This might be something to specifically mention in docs / to BOPS though ("you probably want to buffer or scale the geometry to give a better user experience for those on the edges of areas").
2 participants
Introduces an optional prop
clipGeojsonDatathat can be set to disable a user panning/navigating outside of their own council.I originally was trying to apply a clipping/mask layer but I think that requires more style tinkering than its worth in our case, and this simpler option of applying an extent to the viewport should satisfy what we need !
See these docs / examples to compare:
Next steps:
clipGeojsonDatabased on the existingteams.setting.boundarygeojson if it existsplanx-core)Ideally this can sit in user-testing while Jess is out this next week, then get finalised/merged next week. At which point we could also decide to revisit if our current storage of
teams.settings.boundaryis fine here or if we want to tweak that too in the background.