Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataGrid] Add resetPageOnSortFilter prop that resets the page after sorting/filtering #16450

Merged
merged 13 commits into from
Feb 13, 2025

Conversation

arminmeh
Copy link
Contributor

@arminmeh arminmeh commented Feb 4, 2025

@arminmeh arminmeh added component: data grid This is the name of the generic UI component, not the React module! feature: Filtering Related to the data grid Filtering feature feature: Sorting Related to the data grid Sorting feature feature: Pagination Related to the data grid Pagination feature enhancement This is not a bug, nor a new feature needs cherry-pick The PR should be cherry-picked to master after merge v7.x feature: Server integration Better integration with backends, e.g. data source labels Feb 4, 2025
@arminmeh arminmeh requested a review from a team February 4, 2025 11:43
@arminmeh arminmeh changed the title [DataGrid] Add resetPageAfterSortingOrFiltering prop that resets the page after sorting/filtering [DataGrid] Add resetPageOnSortFilter prop that resets the page after sorting/filtering Feb 12, 2025
@@ -8,6 +8,10 @@ The example below demonstrates how to achieve server-side filtering.

{{"demo": "ServerFilterGrid.js", "bg": "inline"}}

:::info
Combine server-side filtering with [Server-side sorting](/x/react-data-grid/sorting/#server-side-sorting) and [Server-side pagination](/x/react-data-grid/pagination/#server-side-pagination) to avoid fetching more data than needed, since you already process the data outside of the Data Grid.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following text would be more relatable for me. Also, I'd like to keep it as warning, as to me this call-out is not for an "improved" but a "proper" implementation.

Suggested change
Combine server-side filtering with [Server-side sorting](/x/react-data-grid/sorting/#server-side-sorting) and [Server-side pagination](/x/react-data-grid/pagination/#server-side-pagination) to avoid fetching more data than needed, since you already process the data outside of the Data Grid.
To have a proper server-side binding, you should combine server-side filtering with [Server-side sorting](/x/react-data-grid/sorting/#server-side-sorting) and [Server-side pagination](/x/react-data-grid/pagination/#server-side-pagination).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the text updated would warrant IMO an update to a warning callout. As I am still more in favor of keeping it as info I would also like to keep the text as is.

arminmeh and others added 5 commits February 13, 2025 16:16
Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com>
Co-authored-by: Sam Sycamore <71297412+mapache-salvaje@users.noreply.github.com>
Signed-off-by: Armin Mehinovic <4390250+arminmeh@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+mapache-salvaje@users.noreply.github.com>
Signed-off-by: Armin Mehinovic <4390250+arminmeh@users.noreply.github.com>
Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

expect(apiRef.current!.state.pagination.paginationModel.page).to.equal(1);

act(() => {
apiRef.current?.sortColumn('id', 'desc');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ! throughout instead of ??

Suggested change
apiRef.current?.sortColumn('id', 'desc');
apiRef.current!.sortColumn('id', 'desc');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to do this for the whole repo in another PR

@arminmeh arminmeh enabled auto-merge (squash) February 13, 2025 19:09
@arminmeh arminmeh merged commit 510a56e into mui:master Feb 13, 2025
16 checks passed
Copy link

Cherry-pick PRs will be created targeting branches: v7.x

github-actions bot pushed a commit that referenced this pull request Feb 13, 2025
…r sorting/filtering (#16450)

Signed-off-by: Armin Mehinovic <4390250+arminmeh@users.noreply.github.com>
Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com>
Co-authored-by: Sam Sycamore <71297412+mapache-salvaje@users.noreply.github.com>
@arminmeh arminmeh deleted the reset-page-opt-in branch February 13, 2025 19:28
arminmeh added a commit that referenced this pull request Feb 14, 2025
…r sorting/filtering (#16450)

Signed-off-by: Armin Mehinovic <4390250+arminmeh@users.noreply.github.com>
Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com>
Co-authored-by: Sam Sycamore <71297412+mapache-salvaje@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature feature: Filtering Related to the data grid Filtering feature feature: Pagination Related to the data grid Pagination feature feature: Server integration Better integration with backends, e.g. data source feature: Sorting Related to the data grid Sorting feature needs cherry-pick The PR should be cherry-picked to master after merge v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants