fix: DH-22976: re-apply the quickFilters prop on change - #2712
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2712 +/- ##
==========================================
+ Coverage 51.10% 51.52% +0.41%
==========================================
Files 793 797 +4
Lines 45204 45871 +667
Branches 11703 11914 +211
==========================================
+ Hits 23103 23633 +530
- Misses 22055 22219 +164
+ Partials 46 19 -27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates IrisGrid (in packages/iris-grid) so quickFilters can be treated as a controlled prop: when the parent changes quickFilters, the grid re-applies that value (similar to the existing controlled behavior for sorts).
Changes:
- Detect
quickFiltersprop changes incomponentDidUpdateand re-apply them to grid state. - Add an
updateQuickFiltershelper to update state + trigger filtering UI/refresh.
| if (quickFilters !== prevProps.quickFilters) { | ||
| this.updateQuickFilters(quickFilters); | ||
| } |
| this.setState({ | ||
| quickFilters: quickFilters != null ? new Map(quickFilters) : new Map(), | ||
| }); |
There was a problem hiding this comment.
As Copilot mentions, we shouldn't be cloning this map. We should just use the map as is.
| this.setState({ | |
| quickFilters: quickFilters != null ? new Map(quickFilters) : new Map(), | |
| }); | |
| this.setState({ | |
| quickFilters: quickFilters ?? EMPTY_MAP, | |
| }); |
| if (sorts !== prevProps.sorts) { | ||
| this.updateSorts(sorts); | ||
| } | ||
| if (quickFilters !== prevProps.quickFilters) { |
There was a problem hiding this comment.
We should add some unit tests.
Another thing of note - if we apply input filter changes (checked above), those update the quick filters; if quick filters change as well, then we just replace those changes that were just applied from the input filters. I think that's probably okay, and we're unlikely to get a conflict like that.
| updateQuickFilters(quickFilters: ReadonlyQuickFilterMap | null): void { | ||
| this.startLoading('Filtering...', { resetRanges: true }); | ||
| this.setState({ | ||
| quickFilters: quickFilters ?? EMPTY_MAP, | ||
| }); | ||
| this.grid?.forceUpdate(); | ||
| } |
…ters prop actually differs
| if (sorts !== prevProps.sorts && !deepEqualEs6(sorts, prevProps.sorts)) { | ||
| this.updateSorts(sorts); | ||
| } | ||
| if (quickFilters !== prevProps.quickFilters) { | ||
| if ( | ||
| quickFilters !== prevProps.quickFilters && | ||
| !deepEqualEs6(quickFilters, prevProps.quickFilters) | ||
| ) { |
There was a problem hiding this comment.
We shouldn't need deepEquals checks here - if the object is different, then we should apply those changes.
Make IrisGrid re-apply the quickFilters prop on change, so the parent can own/control it. Same as with sorts.