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.

Sign up for GitHub

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

Jump to bottom

[DataGridPro] Make Row reordering work with pagination #15355

Merged
merged 16 commits into from
Dec 7, 2024
Merged

[DataGridPro] Make Row reordering work with pagination #15355

merged 16 commits into from
Dec 7, 2024

Conversation

k-rajat19
Copy link
Contributor

@k-rajat19 k-rajat19 commented Nov 9, 2024 •
edited by MBilalShafi
Loading

Fixes #15351

getRowIndexRelativeToVisibleRows method gives index relative to the current page causing the row-reordering to not work with pages > first page. This PR adds a new lookup to fix it.

Before: https://codesandbox.io/p/sandbox/row-reorder-v5r5q9
After: https://codesandbox.io/p/sandbox/mui-mui-x-x-data-grid-forked-4m7hkp

@mui-bot
Copy link

mui-bot commented Nov 9, 2024 •
edited
Loading

Deploy preview: https://deploy-preview-15355--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 9059c58

@k-rajat19 k-rajat19 marked this pull request as ready for review November 10, 2024 17:33
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Nov 11, 2024
@michelengelen michelengelen added bug 🐛 Something doesn't work feature: Reordering Related to the data grid Reordering feature needs cherry-pick The PR should be cherry-picked to master after merge v7.x labels Nov 11, 2024
MBilalShafi
MBilalShafi reviewed Nov 12, 2024
View reviewed changes
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.

Thank you @k-rajat19 for your contribution.
I added a few comments.
Also, Is it possible to add some test coverage for the bugfix?

packages/x-data-grid-pro/src/hooks/features/rowReorder/useGridRowReorder.tsx Show resolved Hide resolved
packages/x-data-grid-pro/src/hooks/features/rowReorder/useGridRowReorder.tsx Outdated Show resolved Hide resolved
packages/x-data-grid-pro/src/hooks/features/rowReorder/useGridRowReorder.tsx Outdated Show resolved Hide resolved
@k-rajat19 k-rajat19 force-pushed the issue-15351 branch 2 times, most recently from 9dd1a85 to 27e3595 Compare November 17, 2024 19:17
MBilalShafi
MBilalShafi reviewed Nov 26, 2024
View reviewed changes
Copy link
Member

@MBilalShafi MBilalShafi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for fixing the case with the filtered data, is it possible to add some test coverage to avoid it being introduced again?
Or would it make sense to extract this specific fix in another PR and add the test along with that?

packages/x-data-grid-pro/src/hooks/features/rowReorder/useGridRowReorder.tsx Show resolved Hide resolved
packages/x-data-grid/src/hooks/features/sorting/gridSortingSelector.ts Show resolved Hide resolved
@k-rajat19
Copy link
Contributor Author

Thank you for fixing the case with the filtered data, is it possible to add some test coverage to avoid it being introduced again? Or would it make sense to extract this specific fix in another PR and add the test along with that?

A PR has already opened #15455 for that , if you want I can add those changes here otherwise we can do this in that PR after this one lands.

MBilalShafi
MBilalShafi approved these changes Dec 7, 2024
View reviewed changes
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.

Thank you. LGTM. 🎉

packages/x-data-grid/src/hooks/features/sorting/gridSortingSelector.ts Outdated Show resolved Hide resolved
@k-rajat19 @MBilalShafi
Update packages/x-data-grid/src/hooks/features/sorting/gridSortingSel…
9059c58 
…ector.ts

Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com>
Signed-off-by: Rajat  <rd.rajat23@gmail.com>
@MBilalShafi MBilalShafi enabled auto-merge (squash) December 7, 2024 08:15
@MBilalShafi MBilalShafi merged commit 3034df3 into mui:master Dec 7, 2024
16 checks passed
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 7, 2024

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

github-actions bot pushed a commit that referenced this pull request Dec 7, 2024
@k-rajat19 @MBilalShafi @web-flow
[DataGridPro] Make Row reordering work with pagination (#15355)
7c82897 
Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com>
@github-actions github-actions bot mentioned this pull request Dec 7, 2024
Merged
@k-rajat19 k-rajat19 deleted the issue-15351 branch December 7, 2024 09:00
@k-rajat19 k-rajat19 mentioned this pull request Dec 9, 2024
Open
1 task
LukasTy pushed a commit to LukasTy/mui-x that referenced this pull request Dec 19, 2024
@k-rajat19 @MBilalShafi @LukasTy
[DataGridPro] Make Row reordering work with pagination (mui#15355)
34fc3fa 
Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MBilalShafi MBilalShafi MBilalShafi approved these changes

@cherniavskii cherniavskii Awaiting requested review from cherniavskii

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Reordering Related to the data grid Reordering feature needs cherry-pick The PR should be cherry-picked to master after merge v7.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[data grid] Row reordering does not work with pagination
5 participants
@k-rajat19 @mui-bot @MBilalShafi @michelengelen @zannager