feat(country-map): add cross-filters support #35859
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
dosubot
bot
added
dashboard:cross-filters
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/index.js | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
There was a problem hiding this comment.
Pull Request Overview
This PR adds interactive chart behaviors (cross-filtering, drill-to-detail, and drill-by) to the legacy country map chart plugin, along with zoom/pan functionality and visual highlighting for selected regions.
Key Changes:
- Added support for cross-filtering, drill-to-detail, and drill-by behaviors
- Implemented zoom and pan functionality with state persistence across re-renders
- Added visual feedback for selected regions with opacity and stroke styling
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| transformProps.js | Extracts and passes new interactive props (hooks, filterState, emitCrossFilters, entity, inContextMenu) to the chart component |
| index.js | Declares InteractiveChart, DrillToDetail, and DrillBy behaviors in chart metadata to enable interactive features |
| CountryMap.js | Implements interactive behaviors including click handlers for cross-filtering, context menu for drill operations, zoom/pan with bounds, and visual highlighting of selected regions |
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
Outdated
Show resolved
Hide resolved
| const colorFn = feature => { | ||
| if (!feature?.properties) return 'none'; | ||
| const iso = feature.properties.ISO; | ||
| return colorMap[iso] || '#FFFEFE'; |
There was a problem hiding this comment.
The default color '#FFFEFE' for regions without data is nearly white and may be difficult to distinguish from the background, especially in light themes. Consider using a more neutral color like '#E0E0E0' (light gray) to make it clearer which regions have no data.
| return colorMap[iso] || '#FFFEFE'; | |
| return colorMap[iso] || '#E0E0E0'; |
|
|
||
| // Click handler | ||
| const handleClick = feature => { | ||
| if (!emitCrossFilters || typeof setDataMask !== 'function') return; |
There was a problem hiding this comment.
Missing safety check: If entity prop is undefined or null, the filter operations will fail. Add a guard check at the beginning of the function: if (!entity || typeof setDataMask !== 'function') return;
| if (!emitCrossFilters || typeof setDataMask !== 'function') return; | |
| if (!emitCrossFilters || typeof setDataMask !== 'function' || !entity) return; |
| // Store zoom state per chart instance | ||
| const zoomStates = {}; |
There was a problem hiding this comment.
The comment "Store zoom state per chart instance" describes the purpose of zoomStates but there's no cleanup mechanism for this global object. When charts are removed, their zoom states will persist in memory causing a memory leak. Consider adding a cleanup function or using a WeakMap if possible.
| }, | ||
| }); | ||
|
|
||
| highlightSelectedRegion(); |
There was a problem hiding this comment.
Potential issue: highlightSelectedRegion() is called within the handleClick function after calling setDataMask(), but the filterState won't be updated immediately. The highlighting will use stale filterState values. Consider either passing the new selection to highlightSelectedRegion() or ensuring it reads from the updated state, or rely on the component re-rendering to call highlighting.
| .on('zoom', () => { | ||
| const { translate, scale } = d3.event; // [tx, ty] | ||
| let [tx, ty] = translate; | ||
|
|
||
| const scaledW = width * scale; | ||
| const scaledH = height * scale; | ||
| const minX = Math.min(0, width - scaledW); | ||
| const maxX = 0; | ||
| const minY = Math.min(0, height - scaledH); | ||
| const maxY = 0; | ||
|
|
||
| // clamp | ||
| tx = Math.max(Math.min(tx, maxX), minX); | ||
| ty = Math.max(Math.min(ty, maxY), minY); | ||
|
|
||
| g.attr('transform', `translate(${tx}, ${ty}) scale(${scale})`); | ||
| zoomStates[chartKey] = { scale, translate: [tx, ty] }; | ||
| }); |
There was a problem hiding this comment.
Performance issue: The zoom event handler runs on every zoom/pan event without throttling or debouncing. For large maps with many regions, this could cause performance issues. Consider throttling the zoom updates using d3.event.type === 'zoomend' or implementing a throttle mechanism.
There was a problem hiding this comment.
I tried using 'zoomend' and throttling to improve performance, but it made the zoom reset after clicks. I decided to keep the original zoom handler and simply update the WeakMap when the transform actually changes, which keeps zoom smooth and avoids memory issues.
|
|
||
| // Cross-filter support | ||
| const getCrossFilterDataMask = source => { | ||
| const selected = filterState.selectedValues || []; |
There was a problem hiding this comment.
Potential runtime error: filterState.selectedValues will throw an error if filterState is undefined or null. Consider using optional chaining: const selected = filterState?.selectedValues || [];
| const selected = filterState.selectedValues || []; | |
| const selected = filterState?.selectedValues || []; |
| onContextMenu(pointerEvent.clientX, pointerEvent.clientY, { | ||
| drillToDetail: drillToDetailFilters, | ||
| crossFilter: getCrossFilterDataMask(feature), | ||
| drillBy: { filters: drillByFilters, groupbyFieldName: 'entity' }, |
There was a problem hiding this comment.
The drillBy configuration uses a hardcoded string 'entity' for groupbyFieldName, but it should use the actual entity variable from props which represents the column name being used. Change to: drillBy: { filters: drillByFilters, groupbyFieldName: entity }
| drillBy: { filters: drillByFilters, groupbyFieldName: 'entity' }, | |
| drillBy: { filters: drillByFilters, groupbyFieldName: entity }, |
- Added safety checks for missing props (entity, filterState) - Replaced near-white default fill with neutral gray (#d9d9d9) - Used WeakMap to store zoom state per element to avoid memory leaks - Optimized zoom handling by only updating WeakMap when transform changes - Ensured cross-filter highlights use up-to-date selection state - Added guards in click/context menu handlers - Minor code cleanup and readability improvements
1 participant
SUMMARY
This PR adds cross-filters support to the legacy CountryMap plugin with some changes to the map interaction UX:
Note: While this behavior is not present in other Superset charts, I found it helpful. I welcome guidance from reviewers on whether to keep it or remove it for consistency.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION