feat: optionally show a search bar when drawing on an OS basemap to position the map view#618
Conversation
✅ Deploy Preview for oslmap ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| coordinates: [], | ||
| }, | ||
| }; | ||
| clipGeojsonData: GeoJSONFeature | undefined = undefined; |
There was a problem hiding this comment.
Lit Object types have always been a bit weird with default value handling, but this feels a lot more semantically meaningful now and should be functioning exactly the same (eg snap feature which has good unit test coverage also relies on clip extent).
| this.showOSSearch && | ||
| this.drawMode && | ||
| ["OSVectorTile", "OSRaster"].includes(this.basemap) && | ||
| (Boolean(this.osApiKey) || Boolean(this.osProxyEndpoint)); |
There was a problem hiding this comment.
"Showing search" isn't as simple as setting only the showOSSearch prop, we also want to confirm that these other conditions are met for the best UI/UX.
This should avoid a few weird edge cases which could otherwise be created, for example:
- Showing search on a static map, not in draw mode
- Showing OS-based search on a draw mode map using Mapbox Satellite basemap
There was a problem hiding this comment.
Makes sense to me...!
We have a few similar guards here where props need to be grouped. If this was a pure React component it would be a good case for a discriminated union of props to enforce this, but I'm not too sure how we'd...
- Set that up in Lit
- Enforce this in a consumer application (exporting prop types somewhere?)
One to have a look at (timeboxed) in future maybe.
| @@ -1,3 +1,5 @@ | |||
| @import "govuk-frontend/dist/govuk/index"; | |||
There was a problem hiding this comment.
Now required for styling govuk error classes.
| // display "none" ensures always present in DOM, which means role="status" will work for screenreaders | ||
| if (errorEl) errorEl.style.display = "none"; | ||
| if (errorEl && this._searchError) errorEl.style.display = ""; | ||
| } |
There was a problem hiding this comment.
This follows the pattern established by postcode-search and address-autocomplete, so should hopefully be a11y compliant come our next audit 🤞
| this.showOSSearch && | ||
| this.drawMode && | ||
| ["OSVectorTile", "OSRaster"].includes(this.basemap) && | ||
| (Boolean(this.osApiKey) || Boolean(this.osProxyEndpoint)); |
There was a problem hiding this comment.
Makes sense to me...!
We have a few similar guards here where props need to be grouped. If this was a pure React component it would be a good case for a discriminated union of props to enforce this, but I'm not too sure how we'd...
- Set that up in Lit
- Enforce this in a consumer application (exporting prop types somewhere?)
One to have a look at (timeboxed) in future maybe.
There was a problem hiding this comment.
Great to see this included 👍
| drawPointer="dot" | ||
| showOSSearch | ||
| basemap="OSVectorTile" | ||
| osVectorTilesApiKey="${osApiKey}" |
There was a problem hiding this comment.
I actually can't get this to render on the Netlify env, or locally, when I pass in either a key or proxy endpoint.
This might be a problem with how I originally set this up though, the same applied to other stories 🤔
Were you able to get this to work locally?
There was a problem hiding this comment.
Was also not able to get Storybook working locally, even with API key as plain string passed in, but not uniuqe to this new story for me! Happy to come back to this as a follow-up, meant to similarly point out here!
Local dev & index.html examples were completely fine/working as usual, so not anticipating issues with FindProperty integration. But will be an alpha release regardless, justt in case!
2 participants
Changes / how this works
showOSSearchwhich will render the geocode autocomplete search component above the mapdrawMany, drawpolygonetc)Next steps
Prior art
Originally started here theopensystemslab/planx-new#6455, but ultimately couldn't hack two-way data flow and Lit's
updatelife cycle so opted for this "composite component" approach directly withinrenderand controlled via map props instead ➕