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

[DataGridPro] Make Row reordering work with pagination #15355

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

k-rajat19
Copy link
Contributor

@k-rajat19 k-rajat19 commented Nov 9, 2024

Closes #15351

getRowIndexRelativeToVisibleRows method gives index relative to the current page but I think using the absolute index would help in all cases, added a getRowIndex method which gives the absolute index.

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

@mui-bot
Copy link

mui-bot commented Nov 9, 2024

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

Generated by 🚫 dangerJS against 9025208

}, {}),
[rows],
);
const getRowIndex = React.useCallback((id: GridRowId) => lookup[id], [lookup]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we add this method to the GridRowApi ?

@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
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?

Copy link
Member

Choose a reason for hiding this comment

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

I noticed it doesn't work as expected with filtered data.

row-reorder.mp4

Could we make it work properly?

@@ -63,6 +64,16 @@ export const useGridRowReorder = (
const ownerState = { classes: props.classes };
const classes = useUtilityClasses(ownerState);
const [dragRowId, setDragRowId] = React.useState<GridRowId>('');
const rows = gridExpandedSortedRowEntriesSelector(apiRef);
Copy link
Member

Choose a reason for hiding this comment

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

Using a selector directly in the React component might yield bugs since it'll not ensure proper reactivity.
Wrapping the selector with useGridSelector is preferred as it ensures to re-render the component when the state value updates.

@@ -63,6 +64,16 @@ export const useGridRowReorder = (
const ownerState = { classes: props.classes };
const classes = useUtilityClasses(ownerState);
const [dragRowId, setDragRowId] = React.useState<GridRowId>('');
const rows = gridExpandedSortedRowEntriesSelector(apiRef);
const lookup = React.useMemo(
Copy link
Member

@MBilalShafi MBilalShafi Nov 12, 2024

Choose a reason for hiding this comment

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

I'd prefer to off-load this to a selector as it'll remove the need for a memoization as well as ensure proper reactivity.
Wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

[data grid] Row reordering does not work with pagination
5 participants