-
Notifications
You must be signed in to change notification settings - Fork 1
MapFeaturesContext: Architecture Concept #320
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
base: develop
Are you sure you want to change the base?
MapFeaturesContext: Architecture Concept #320
Conversation
👷 Deploy request for gbof-c19nyc-staging pending review.Visit the deploys page to approve it
|
👷 Deploy request for pss-scavenger-hunt pending review.Visit the deploys page to approve it
|
👷 Deploy request for juel-staging pending review.Visit the deploys page to approve it
|
👷 Deploy request for padp-staging pending review.Visit the deploys page to approve it
|
| <div | ||
| className='flex grow h-[calc(100vh-160px)]' | ||
| > | ||
| <MapFeaturesContextProvider> |
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.
Note: no changes in this component, except for wrapping everything in MapFeaturesContextProvider.
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.
Would the MapSearch component be a more appropriate place for this context than the MapLayout? Or was there a reason for specifically putting this in MapLayout?
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.
It also looks like there were come changes to CSS classes?
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 component that's rendering the selected geometry overlay is the <BasePanel> which, if I saw correctly, is rendered in <SearchRoutes> – which is why <MapLayout> would be the lowest component in the hierarchy this would have to be, I think.
Re CSS classes: I'll double check. But I think that's just an artefact of how git interprets the change of wrapping the whole markup.
| if (props.resolveGeometry && item) { | ||
| const highRes = props.resolveGeometry(item); | ||
| updatePlace(highRes); | ||
| } else if (!_.isEmpty(places.filter((place) => place.place_geometry))) { |
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.
TBH I didn't figure out what this branch does. Under what conditions will new places come in? I assume this code needs changing... instead of creating one FeatureCollection, we probably need to loop through each place and call updatePlace for each one separately?
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.
For non-place records (people, events, etc), the geometry data is obtained from the related place records (fetched above). For this use case, the record could be linked to multiple geometries, so yes, I believe we'd need to loop through each and update.
For place records, we use the resolveGeometry prop to obtain the geometry data (pulled off the base record). This returns a FeatureCollection and it looks like the updatePlace function expects a Feature?
|
@dleadbetter would you have a look at this proposal, and comment, and if you like the approach, provide @rsimon with access to actually implement it? EDIT: By way of context, this is the first step in trying to address the issues seen on the Pan African Data site, where there are several overlapping layers of polygons of differing resolutions.
|
dleadbetter
left a comment
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.
I think this implementation is likely a step in the right direction, but I don't think it is going to completely solve the issue with overlapping polygons (and maybe that's okay/expected?): Although we're replacing the geometry of the selected record with a higher resolution polygon, adjacent polygons will still be lower resolution, and likely to overlap.
We're also going to lose a few features provided by the LocationMarkers component:
- The geometry for the currently selected record will no longer be highlighted a different color (#292)
- For projects that use coordinates instead of polygon geometries, we'll no longer be animating the marker for the currently selected record with a "pulsing" animation
- When selecting a record, we'll no longer be fitting the map bounds to the geometries of the selected record
I'm sure there are other caveats to this approach that I'm missing by just looking at the code, so we'll want to thoroughly test with different datasets.
| <div | ||
| className='flex grow h-[calc(100vh-160px)]' | ||
| > | ||
| <MapFeaturesContextProvider> |
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.
Would the MapSearch component be a more appropriate place for this context than the MapLayout? Or was there a reason for specifically putting this in MapLayout?
| <div | ||
| className='flex grow h-[calc(100vh-160px)]' | ||
| > | ||
| <MapFeaturesContextProvider> |
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.
It also looks like there were come changes to CSS classes?
| if (props.resolveGeometry && item) { | ||
| const highRes = props.resolveGeometry(item); | ||
| updatePlace(highRes); | ||
| } else if (!_.isEmpty(places.filter((place) => place.place_geometry))) { |
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.
For non-place records (people, events, etc), the geometry data is obtained from the related place records (fetched above). For this use case, the record could be linked to multiple geometries, so yes, I believe we'd need to loop through each and update.
For place records, we use the resolveGeometry prop to obtain the geometry data (pulled off the base record). This returns a FeatureCollection and it looks like the updatePlace function expects a Feature?
|
@dleadbetter good points - I missed the problem with the selection highlight color/pulsing marker! So, yes agreed: this suggests we should stick with a separate overlay after all. We could still leverage the same basic approach, though, I guess. But instead of updating the geometry centrally, we could use the That wouldn't solve the issue you mention, of the (high-res) border of the selected place not fitting the (low-res) border of it's neighbours. But it would remove the duplication. If we really wanted to achieve a "level of detail"-type rendering, that would require a more sophisticated approach. Essentially, we'd have to dynamically fetch places as the user zooms. Might be overkill? |
|
Quick follow-up questions:
|
I think part of the problem is that since Typesense uses unstructured data, we're duplicating these large polygons for every record they're related to. For example, if 100 people are related to Russia, we're duplicating the polygon (which is +20MB) 100 times in the Typesense index, causing the search request to take forever. I think a possible solution is as you described, downloading the full data set before hand, but I think we could also just serve the geometry data from somewhere other than Typensense (maybe the CoreData API?). Or at least from server rendered sites we serve the data from an API to cut down on the overhead of maintaining the dataset and for static rendered sites we download the full data set. This would be consistent with how we're building sites currently. |
|
@dleadbetter agreed! Pulling the data once from an API endpoint, and then just referencing the feature IDs in the TS response sounds ideal. What do you think about the utility function in |


In this PR
This is a draft PR that proposes centralizing all map-rendered geo-features into a shared, context-driven state. The goal is to maintain the current behavior (auto-populating the map from TypeSense hits) while enabling other components to update individual places at runtime (e.g. when the
BasePanelfetches higher-resolution feature data).Key Changes
MapFeaturesContextthat internally usesuseCachedHitsto derive a sharedplacesstate.useCachedPlaceshook that exposes:updatePlacemethod for replacing an existing place with an updated (higher-res) versionMapFeaturesContextProviderwithinMapLayout.MapViewto useuseCachedPlacesinstead ofuseCachedHits.LocationMarkersinBasePanelthat previously created duplicate overlays. Instead,BasePanelnow updates the shared places state viaupdatePlace.IMPORTANT: this is all conceptual and untested, since I'm lacking a proper setup (TypeSense access credentials, Tina backend etc.) Consider this a "pseudo-code-ish" proposal intended to illustrate the idea, and for further discussion.