feat: GeoJSONViewer for map subjects#7442
feat: GeoJSONViewer for map subjects#7442eatyourgreens wants to merge 4 commits intozooniverse:mainfrom
Conversation
|
@mcbouslog I don't know if this will be useful, but a map seems more accessible/usable in Talk than the raw subject JSON. |
There was a problem hiding this comment.
Pull request overview
Adds a new GeoJSONViewer to render GeoJSON subjects with an embedded OpenLayers map and show reference_data beneath, plus wiring/styling and a new ol dependency.
Changes:
- Add OpenLayers (
ol) dependency for map rendering. - Add
GeoJSONViewercomponent and integrate it into the file viewer selection / JSON inspection flow. - Add Stylus styles for the GeoJSON viewer and include them in the main CSS bundle.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds ol dependency needed for OpenLayers rendering. |
| package-lock.json | Locks ol and its transitive dependencies. |
| css/main.styl | Includes the new GeoJSON viewer stylesheet. |
| css/geojson-viewer.styl | Adds styles for OpenLayers controls and reference data layout. |
| app/components/file-viewer/json-container.jsx | Routes detected GeoJSON JSON payloads to GeoJSONViewer. |
| app/components/file-viewer/index.jsx | Attempts to select GeoJSONViewer for application/geo+json format. |
| app/components/file-viewer/geojson-viewer.jsx | New OpenLayers-based GeoJSON map viewer implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
15bcd00 to
efb266d
Compare
bbc59a3 to
39e628b
Compare
| let vectorSource; | ||
| let vectorLayer; | ||
| try { | ||
| vectorSource = new VectorSource({ useSpatialIndex: false }); |
There was a problem hiding this comment.
This errors with a stack overflow (too much recursion) in RBush, even though spatial indexing is disabled, and I can't see why.
There was a problem hiding this comment.
This was an error in the webpack config, only triggered when you have two modules in the tree with ambiguous names (in this case RBush.js and rbush.js on case-insensitive MacOS.)
Fixed in this PR by adding stricter module resolution rules to the webpack configs.
3af916b to
831e774
Compare
f4850e8 to
3d0e952
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If detected as GeoJSON (or JSON with type === 'Feature' or 'FeatureCollection'), use GeoJSONViewer | ||
| if (props.format === 'geo+json' || jsonData?.type === 'Feature' || jsonData?.type === 'FeatureCollection') { | ||
| return <GeoJSONViewer {...props} jsonData={jsonData} />; |
There was a problem hiding this comment.
There are existing component tests under app/components/file-viewer (e.g. canvas-viewer.spec.jsx), but the new GeoJSON rendering path isn’t covered. Please add tests that verify GeoJSON detection/routing and that the viewer renders a map + reference data for a Feature/FeatureCollection payload.
| if (extent) { | ||
| view.fit(extent, { | ||
| padding: [100, 100, 100, 100], | ||
| maxZoom: 12, | ||
| }); |
There was a problem hiding this comment.
view.fit(...) is being called during render, which will re-fit whenever this component re-renders (e.g. parent updates), preventing users from panning/zooming. Move the fit call into a useEffect that runs when the map becomes available / when features change, and guard it with a non-empty extent check (empty VectorSource#getExtent() can still be truthy).
There was a problem hiding this comment.
FittedVectorLayer only re-renders when features changes, so this should be fine.
| module: { | ||
| rules: [{ | ||
| test: /\.m?js$/, | ||
| include: /node_modules\/react-openlayers\/dist/, |
There was a problem hiding this comment.
This include regex hard-codes POSIX path separators (/node_modules/.../dist/), so the rule won’t match on Windows where paths use \\. Use a cross-platform pattern (e.g. [\\/]node_modules[\\/]react-openlayers[\\/]dist[\\/]) or compute an absolute include path with path.dirname(require.resolve(...)).
| include: /node_modules\/react-openlayers\/dist/, | |
| include: /[\\/]node_modules[\\/]react-openlayers[\\/]dist[\\/]/, |
| module: { | ||
| rules: [{ | ||
| test: /\.m?js$/, | ||
| include: /node_modules\/react-openlayers\/dist/, |
There was a problem hiding this comment.
This include regex hard-codes POSIX path separators (/node_modules/.../dist/), so the rule won’t match on Windows where paths use \\. Use a cross-platform pattern (e.g. [\\/]node_modules[\\/]react-openlayers[\\/]dist[\\/]) or compute an absolute include path with path.dirname(require.resolve(...)).
| include: /node_modules\/react-openlayers\/dist/, | |
| include: /[\\/]node_modules[\\/]react-openlayers[\\/]dist[\\/]/, |
3d0e952 to
7ec8e95
Compare
There was a problem hiding this comment.
ol is ESM, so mocha needs a custom loader to resolve partial specifiers like ol/layer/Vector to their fully-specified paths: ol/layer/Vector.js.
|
@eatyourgreens - this is awesome, thank you!! Let's please keep this open to revisit with parity in FEM and use as a reference, but I'm not sure if we'll review/merge in the short-term. Just letting you know timing. I can/will help with merge conflicts. |
| const features = useMemo(() => geoJsonFormat.readFeatures(jsonData, { | ||
| dataProjection: 'EPSG:4326', // incoming data is WGS 84. | ||
| featureProjection: 'EPSG:3857', // rendered map is Web Mercator. | ||
| featureClass: RenderFeature // render read-only features on the map. |
There was a problem hiding this comment.
@mcbouslog this is kind of hidden away in the documentation, but OpenLayers exports a RenderFeature class for cases where you want to show features but don't need them to be interactive eg. read-only locations.
|
@mcbouslog no worries. It occurred to me that the first thing volunteers will ask for, on mapping projects, is to see the maps in Talk. |
A `GeoJSONViewer` component that displays GeoJSON subjects as an embedded OpenLayers map, with the subject reference data beneath. Supports `.geojson` files with the `application/geo+json` MIME type, and falls back to the default JSON viewer for other JSON files.
c053974 to
22eefe8
Compare
A
GeoJSONViewercomponent that displays GeoJSON subjects as an embedded OpenLayers map, with the subject reference data beneath.Supports
.geojsonfiles with theapplication/geo+jsonMIME type, and falls back to sniffing file content forFeatureorFeatureCollectionfor generic JSON files.Builds the map with these
react-openlayerscomponents:Examples
Required Manual Testing
Review Checklist
npm ciand app works as expected?Optional
ChangeListenerorPromiseRenderercomponents with code that updates component state?