feat: extend goto feature to work with heatmaps row, column, and global extremas using fill values#569
Conversation
|
PR Review: feat/heatmape_extrema - Go to Extrema in Heatmap Good contribution! Adding extrema navigation to heatmap fills a clear gap. The structure follows existing patterns in bar.ts and segmented.ts. Here are the findings: BUGS
findGlobalExtrema checks heatmapValues.length === 0 but not whether heatmapValues[0] is empty. If heatmapValues[0] is [], then extremaValue = heatmapValues[0][0] initializes as undefined and all comparisons silently fail, returning { row: 0, col: 0, value: undefined }. findColExtrema has the same issue: it bounds-checks colIndex against heatmapValues[0].length but never verifies heatmapValues[0] itself exists.
If heatmapValues contains NaN (not explicitly forbidden by HeatmapData), comparisons like value > extremaValue always return false. The initial cell is silently returned as the extrema regardless of actual data. Guard against NaN — MathUtil.safeMin/safeMax shows the established pattern. PERFORMANCE
getExtremaTargets() calls findGlobalExtrema('max') then findGlobalExtrema('min') separately, each scanning the full grid. A single combined pass — as MathUtil.minMax() demonstrates for 1D arrays — could return both positions at once. This runs on every go-to-extrema invocation. CODE QUALITY
navigateToExtrema uses target.groupIndex / target.categoryIndex, not target.pointIndex. This diverges from bar.ts where this.col = target.pointIndex drives navigation directly. A comment clarifying that pointIndex is set only to satisfy the ExtremaTarget interface would help future readers.
The interface has an optional display field documented as 'Structured display data for UI rendering. Avoids string parsing in UI components.' Worth checking whether the UI parses label strings downstream; if so, populating display is the correct fix.
Extract to a local variable: TESTS e2e_tests/specs/heatmap.spec.ts has no extrema coverage. At minimum, a test that triggers the go-to-extrema dialog, navigates to global max/min, and verifies the cursor position and announcement would prevent regressions. SUMMARY Severity | Item The core approach is correct and well-structured. Addressing the two bugs and the redundant traversal before merging would make this solid. |
|
Works nicely and looking good to me |
|
🎉 This PR is included in version 3.56.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Pull Request
Description
Added identification and navigation to global, row, and column extrema points in heatmap based on fill values. Earlier, heatmap did not support the Go to feature.
Related Issues
Changes Made
◦ findGlobalExtrema() - finds global max/min across entire grid
◦ findRowExtrema() - finds max/min in a specific row
◦ findColExtrema() - finds max/min in a specific column
Screenshots (if applicable)
Checklist
ManualTestingProcess.md, and all tests related to this pull request pass.Additional Notes