feat: add bidirectional map/table selection sync to collapsible data table [PR2] - #3720
BRaimbault wants to merge 8 commits into
Conversation
|
🚀 Deployed on https://pr-3720.maps.netlify.dhis2.org |
1489b0f to
782301d
Compare
782301d to
2e5fe4f
Compare
There was a problem hiding this comment.
In general this looks good. I did leave some comments which you can take or leave.
Another thing I noticed while reviewing was a general pattern where I saw:
- A diff in the prop-types
- A diff in the props in the function signature
- No changes in the function body
- A diff in the JSX where the added props being passed down to descendants
So this is prop-drilling and you could consider addressing it by introducing a/multiple context providers and maybe some hooks. But this is just a casual comment. I didn't look into it deeply and also realise this is a codebase with a mix of function and class components, so it could get messy.
The merge-base changed after approval.
2e5fe4f to
14df4cc
Compare
14df4cc to
6d9fa0b
Compare
6d9fa0b to
e6143ed
Compare
|
Thanks for your review @HendrikThePendric, I addressed your specific comments and marked them as resolved. On the prop-drilling observation, agreed, but don't want to extend this PR's scope now. I will explore using a context as a PR stack clean-up or at a later stage. |
9cc47fc to
70bcd59
Compare
Clarify/relocate the boundsDependency comment in useTableData, and rename Layer's componentDidUpdate handlers from the misleading handleXChange pattern (reads as event handlers, when most of the work is detecting whether a change is relevant) to a consistent sync* naming scheme, per review feedback.
70bcd59 to
d9d782f
Compare
|



Part of DHIS2-21456
Description
Adds bidirectional highlighting/selection sync between the map and data table, persistent multi-feature selection, and a collapsible/resizable table panel. Second in the data table improvement series, building on PR1's toolbar and context menu.
Selection (DataTable)
selectionreducer, scoped per layer, and resets on layer switchMap ↔ table sync
@dhis2/ui's native greytr:hoverstyle while map-hover showed our own blue highlight)Filtering
Zoom actions
Table panel (BottomPanel)
Bug fixes
.includes()scans in selection filtering replaced with O(1)SetlookupsmapStateToPropswhen no context menu is openmaps-gl
Cluster.setVisibleIds()override: "show only selected" now re-clusters from the filtered feature set (viasource.setData()) instead of applying a GL filter expression, which can't target cluster circles (they carry no per-feature id). Applies to client- and donut-clustered event layers; server-clustered layers are unaffected since their data table is already unsupported today (unrelated pre-existing limitation)Layer#panToFeature()now accepts an array of ids, fitting the map to their combined boundsQuality checklist
Add N/A to items that are not applicable.
Screenshots
supporting images