fix: DH-21744: Fix Advanced Filters in Pivots#2624
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates IrisGrid advanced-filter rendering to better support pivot/column-by scenarios, including models that expose “special” columns via negative indices.
Changes:
- Extract advanced-filter button and menu rendering into
renderAdvancedFilterButton/renderAdvancedFilterMenu. - Update
focusFilterBarto usegetModelColumnand treat negative column indices as valid when filterable. - Render advanced-filter buttons and popper menus for filterable columns at negative indices (when the model exposes them).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Advanced filter buttons for columns at negative indexes | ||
| // Models can expose columns at negative indexes (e.g., model.columns[-1]) | ||
| for (let i = -1; model.columns[i] != null; i -= 1) { | ||
| if (!model.isFilterable(i)) { | ||
| // eslint-disable-next-line no-continue | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The logic for iterating over negative-index columns (for (let i = -1; model.columns[i] != null; i -= 1)) is duplicated for both the filter-bar buttons and the popper menus. Consider extracting a small helper that yields the filterable negative indices (or caches them per-render) to keep the render method easier to maintain and reduce the chance of these paths diverging.
| // Advanced filter buttons for columns at negative indexes | |
| // Models can expose columns at negative indexes (e.g., model.columns[-1]) | |
| for (let i = -1; model.columns[i] != null; i -= 1) { | |
| if (!model.isFilterable(i)) { | |
| // eslint-disable-next-line no-continue | |
| continue; | |
| } | |
| // Helper for advanced filter buttons for columns at negative indexes. | |
| // Models can expose columns at negative indexes (e.g., model.columns[-1]). | |
| const getFilterableNegativeColumnIndices = (): number[] => { | |
| const indices: number[] = []; | |
| for (let i = -1; model.columns[i] != null; i -= 1) { | |
| if (model.isFilterable(i)) { | |
| indices.push(i); | |
| } | |
| } | |
| return indices; | |
| }; | |
| // Advanced filter buttons for columns at negative indexes | |
| for (const i of getFilterableNegativeColumnIndices()) { |
| // Advanced filter buttons for columns at negative indexes | ||
| // Models can expose columns at negative indexes (e.g., model.columns[-1]) | ||
| for (let i = -1; model.columns[i] != null; i -= 1) { | ||
| if (!model.isFilterable(i)) { | ||
| // eslint-disable-next-line no-continue | ||
| continue; | ||
| } | ||
| const buttonCoordinates = metricState | ||
| ? metricCalculator.getAdvancedFilterButtonCoordinates( | ||
| i, | ||
| metricState, | ||
| metrics | ||
| ) | ||
| : null; | ||
| if (buttonCoordinates != null) { | ||
| filterBar.push( | ||
| this.renderAdvancedFilterButton(i, i, buttonCoordinates) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This change adds new behavior to render advanced-filter buttons/menus for negative column indices (when models expose columns[-1], etc.), but the existing IrisGrid tests only cover the default behavior where negative indices return null coordinates. It would be good to add a test that stubs a metricCalculator implementation returning coordinates for a negative index and verifies the button/menu render and can be shown/hidden correctly for that negative column.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2624 +/- ##
==========================================
- Coverage 49.78% 49.77% -0.01%
==========================================
Files 774 774
Lines 43767 43795 +28
Branches 11256 11085 -171
==========================================
+ Hits 21789 21799 +10
- Misses 21960 21978 +18
Partials 18 18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vbabich
changed the title
1 participant
No description provided.