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] Added a guard for reorder cells #11159

Merged

Conversation

michelengelen
Copy link
Member


Prevents disableColumnReorder prop from disabling dragevents on the reorder cell.

Fixes #11126

@michelengelen michelengelen added 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 labels Nov 23, 2023
@michelengelen michelengelen self-assigned this Nov 23, 2023
@@ -277,8 +277,11 @@ const GridRow = React.forwardRef<HTMLDivElement, GridRowProps>(function GridRow(
'width' | 'colSpan' | 'showRightBorder' | 'indexRelativeToAllColumns'
>,
) => {
const isReorderCell = column.field === '__reorder__';
Copy link
Member

Choose a reason for hiding this comment

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

This whole condition is way to complex and hard to read.
IMHO it should be a full method or at list a variable assignment.

It was already too complex before your addition though 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

thats true (😛) ... I'll check if I can extract that into a helper

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I did look around a bit.

const disableDragEvents =
(disableColumnReorder && column.disableReorder) ||
(!rowReordering &&
!!sortModel.length &&
treeDepth > 1 &&
Object.keys(editRowsState).length > 0);

Is basically duplicated in the drag handlers. That seems a bit weird.
My proposal would be to allow the drag events to being published, but handle the event execution like before.
For reference the row reorder handles it here:
const isRowReorderDisabled = React.useMemo((): boolean => {
return !props.rowReordering || !!sortModel.length || treeDepth !== 1;
}, [props.rowReordering, sortModel, treeDepth]);

And the column reorder here:

if (props.disableColumnReorder || params.colDef.disableReorder) {
return;
}

I would still keep the disableDragEvents variable for this case:

const disableDragEvents = Object.keys(editRowsState).length > 0;

@flaviendelangle WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

This can make sense
I'll let @cherniavskii validate because my knowledge of the codebase is rusted.

Copy link
Member

Choose a reason for hiding this comment

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

Adding some context on why we disable drag handlers: #4841

Copy link
Member Author

Choose a reason for hiding this comment

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

Since disabling the dragEvents is beneficial (thanks for pointing that out @cherniavskii) I just renamed and restructured it a tiny bit to make it more readable at least.

@mui-bot
Copy link

mui-bot commented Nov 23, 2023

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

Generated by 🚫 dangerJS against 2e8441e

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Seems like some of the changes above broke a few tests

@michelengelen
Copy link
Member Author

Seems like some of the changes above broke a few tests

@cherniavskii fixed (finally 😅 )

@MBilalShafi MBilalShafi changed the title [data grid] Added a guard for reorder cells [DataGrid] Added a guard for reorder cells Nov 29, 2023
const isEditingRows = Object.keys(editRowsState).length > 0;

const canReorderColumn = !(disableColumnReorder || column.disableReorder);
const canReorderRow = rowReordering || !sortModel.length || treeDepth <= 1 || isEditingRows;
Copy link
Member

Choose a reason for hiding this comment

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

Is this expression correct? I would expect it to be:

 const canReorderRow = rowReordering && !sortModel.length && treeDepth <= 1 && !isEditingRows;

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch ... it was working like this as well ... but the cases where it shouldn't be working did work as well, so that would have been bad! Thanks for catching that!

@michelengelen michelengelen enabled auto-merge (squash) December 1, 2023 08:35
@michelengelen michelengelen merged commit 19dcc41 into mui:next Dec 4, 2023
17 checks passed
@michelengelen michelengelen deleted the data-grid/11126/row-reorder-bug branch May 16, 2024 14:49
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Minor issues with row reordering when disableColumnReorder is true
4 participants