Add polygon, polyline, and rectangle support#67
Conversation
Stores all location types as GeoJSON in a new geometry_json column, with latitude and longitude computed from it for backward-compatible address search. Enables the Leaflet.draw shape tools on the edit form, adds Leaflet.deflate to collapse small shapes at low zoom, and updates the browse map, item show page, exhibit layout, and static site export to render shapes alongside point markers.
'Balloon' is legacy terminology; 'popup' is the standard term used throughout the mapping library. Also switches CSS class names from underscores to hyphens to match standard convention.
Add a gray full-bleed header (label or item title) to browse, item show, and exhibit popups. Browse popup order: header → thumbnail → title link → snippet. Exhibit popup: header → attachment body → title link. Scope CSS with :has() to avoid affecting edit-form popups. Remove theme-inherited dotted border-bottom from popup links.
Store the bounding box center as latitude/longitude on shape _locationData so that _mapForm has coordinates to seed the initial map view when re-rendering after a failed save. Previously, shapes had no lat/lng in _locationData, causing the map to center at 0°N 0°E.
|
I changed the base so this will be easier to look at in the interface. |
Simplify the listener that calls popup.update() after images load, removing the dependency on Leaflet's private _panAnim API. Move it from addMarker into addLayerFromGeometry so shapes get the same treatment as markers.
Moves SR announcement logic from addMarker into addLayerFromGeometry so it covers both markers and shapes. Shapes now announce title, bounding box center coordinates, and open/close status, consistent with markers.
The previous check accepted values like {"type":"Point"} (no coordinates)
or {"type":"LineString","coordinates":[]} which would cause PHP warnings
in beforeSave() and could crash the edit form when loaded.
zerocrates
left a comment
zerocrates
left a comment
There was a problem hiding this comment.
I think this looks pretty good.
Only thing not really in the individual comments is, I have a vague sense that there's maybe more special-casing for Point then there needs to be... I know it needs some stuff like zoom/fit handling but there are some places, like loading in the current values when creating the form, when i would have thought you could just roll in the locations all with the same codepath using the json, rather than needing to call two different methods depending on whether it's a point or shape.
|
|
||
| OmekaMap.prototype = { | ||
|
|
||
| map: null, |
There was a problem hiding this comment.
Why drop these initializations? I feel like it's easier to be able to just see what's available on the map object at a glance.
There was a problem hiding this comment.
Restored them. Done in the "restore map prototype fields" commit.
|
|
||
| link.appendTo(listElement); | ||
| listElement.appendTo(list); | ||
| jQuery.each(this.deflateGroup.getLayers(), function (index, layer) { |
There was a problem hiding this comment.
This looks like it will make the list have all the shapes come after the points, is that what happens?
My assumption is that we don't really need markers as a markers-only array anymore and having it have all the locations makes more sense vs. iterating over two separate structures.
There was a problem hiding this comment.
Yes, the old code listed all points then all shapes. Fixed: this.markers is now this.locations and holds every layer (points and shapes) in load order, so buildListLinks iterates one structure and interleaves them.
| // they cluster alongside point markers. | ||
| this.deflateGroup = L.deflate({ | ||
| minSize: 10, | ||
| markerLayer: this.clusterGroup, |
There was a problem hiding this comment.
This looks like it might break if clustering is disabled... or does null just work as a default here?
| queue_css_file('geolocation-marker', null, null, 'css', $version); | ||
| queue_js_file(['leaflet/leaflet', 'leaflet/leaflet-providers', 'leaflet-draw/leaflet.draw', 'map'], 'javascripts', [], $version); | ||
|
|
||
| if (get_option('geolocation_cluster')) { |
There was a problem hiding this comment.
Why the switch to always load the clustering support?
There was a problem hiding this comment.
Removed that override so the item-show map follows the geolocation_cluster option like the browse map, and reverted _head() to load markercluster only when clustering is enabled.
Addresses review comments 1 and 2 on PR #67. - Restore the at-a-glance property declarations on OmekaMap.prototype (updated to current names: locationBounds, deflateGroup, locations). - Rename this.markers -> this.locations everywhere and push both point markers and shape layers into it, so the sidebar list is built from a single load-ordered structure (interleaving points and shapes) instead of iterating markers then deflate layers (shapes always listed last). buildListLinks branches on L.Marker for click behavior; _geolocationTitle is now set on every browse-map layer for uniform labeling.
Addresses review comments 3 and 4 on PR #67. GeolocationMapSingle hard-coded cluster=true, overriding the admin's geolocation_cluster setting and forcing markercluster to load on every page. Drop the override so the item-show map defaults from the option like the browse map, and load markercluster's CSS/JS only when clustering is enabled (leaflet-deflate still loads unconditionally). Also expand the L.deflate comment to note that markerLayer is safely null when clustering is off (Leaflet.Deflate falls back to its own FeatureGroup).
Addresses the top-level review note on PR #67 about Point special-casing. Collapse OmekaMapForm's separate addLocation(lat,lng,...) and addShape(json,...) into a single addLocation(locationData) that builds either a marker or a shape from geometry_json via L.GeoJSON.geometryToLayer. Loading existing values, the draw CREATED handler, and the geocoder now all call it with a location object instead of branching on point vs shape.
Good call. I unified the edit form's two methods ( I did look at the read-only side ( |
2 participants
Extends location support beyond point markers to include polygons, polylines, and rectangles. Shapes can be drawn, edited, labeled, and deleted on the item edit map, and render natively on browse, item show, and exhibit maps. Existing point locations and the geographic radius search are unaffected.
This release also redesigns map popups to match Omeka S Mapping style, and replaces the term "balloon" with "popup" throughout.