CardView: dataController -> paging in the remote OData DataSource not working #29877
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tucchhaa
reviewed
May 15, 2025
| }; | ||
| }; | ||
|
|
||
| describe('regressions', () => { |
There was a problem hiding this comment.
Can we rename the description to 'DataController' ? I think current name isn't suitable and doesn't follow the way other testing files have their descriptions.
Comment on lines
+238
to
+245
| if (!equalByValue( | ||
| this.previousDisplayFilter ?? null, | ||
| displayFilter ?? null, | ||
| { | ||
| maxDepth: FILTER_OBJ_COMPARE_DEPTH, | ||
| strict: true, | ||
| }, | ||
| )) { |
There was a problem hiding this comment.
Suggested change
| if (!equalByValue( | |
| this.previousDisplayFilter ?? null, | |
| displayFilter ?? null, | |
| { | |
| maxDepth: FILTER_OBJ_COMPARE_DEPTH, | |
| strict: true, | |
| }, | |
| )) { | |
| const filterChanged = !equalByValue( | |
| this.previousDisplayFilter ?? null, | |
| displayFilter ?? null, | |
| { | |
| maxDepth: FILTER_OBJ_COMPARE_DEPTH, | |
| strict: true, | |
| }, | |
| ); | |
| if (filterChanged) { |
Comment on lines
+185
to
+186
| describe('when displayFilter and filter from dataSource are empty', () => { | ||
| it('should return empty filter', () => { |
There was a problem hiding this comment.
Maybe just?:
Suggested change
| describe('when displayFilter and filter from dataSource are empty', () => { | |
| it('should return empty filter', () => { | |
| it('should return empty filter when displayFilter and filter from dataSource are empty', () => { |
I doubt that there will be any other tests in this describe block.
| dataSourceFilter: ['a', '=', 123], | ||
| columnFilterValues: [1, 2], | ||
| }); | ||
| expect(gridCore.getCombinedFilter()).toMatchInlineSnapshot(` |
There was a problem hiding this comment.
can we use toEqual here too? Just to be consistent
| dataSourceFilter: undefined, | ||
| columnFilterValues: [1, 2], | ||
| }); | ||
| expect(gridCore.getCombinedFilter()).toMatchInlineSnapshot('undefined'); |
There was a problem hiding this comment.
can we use toEqual/toBe here too? Is there any specific reason why toMatchInlineSnapshot was used?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR consist of two commits:
fix: bug
Direct fix of the bug described in the card.
Problem: each time some loadOption is changed, we call effect to pass changed values to dataSource and call
.loadif we need it.In case of filtering, we were passing the
displayFilter.value ?? nullto dataSource, but the displayField could also beundefined. In result, we passed each time the 'null' value, but we were comparing it withundefinedeach time.Because of this, filter was set each time when any loadOption was changed, and there's internal dataSource logic to drop pageIndex after filtering is changed (since it could lead to changes in totalCount)
fix: getCombinedFilter
I noticed that we didn't have proper logic for getCombinedFilter and fixed it
When we were syncing cardview's display filter to dataSource filter, we were just calling
dataSource.filter(displayFilter). However, dataGrid behaves in different way:Filter, specified in DataSource, is never changed, and is used as 'initial top-level filter'. When we set filter by UI, displayFilter (called additionalFilter in DataGrid) was combined to filter in DataSource, not overwrite it.
I added combineFilter method, used it in
customizeStoreLoadOptionscallback and fixed public methodgetCombinedFilter