[XGrid] Add ability to disable reorder on some columns #2085

flaviendelangle · 2021-07-06T14:37:10Z

Closes #1161
Closes #1768

Implemented behavior :

A column with disableReorder: true can't be drag&dropped
If the first visible column of the grid has disableReorder: true, we can't drag&drop a column left of it
If the last visible column of the grid has disableReorder: true, we can't drag&drop a column right of it

Right now the behavior is at the drag&drop level, I do not block the updates made with apiRef.
Do we want to do it?

Do you prefer?

  /**
   * If `true`, this column can not be reordered
   * @default false
   */
  disableReorder?: boolean;

OR

  /**
   * If `true`, this column can be reordered
   * @default true
   */
  orderable?: boolean;

We have both (disableExport but resizable), which is not great from a user perspective I think.

I did not implement the behavior on the column panel yet. Preview: https://deploy-preview-2085--material-ui-x.netlify.app/components/data-grid/columns/#column-reorder.

oliviertassinari · 2021-07-06T17:37:05Z

For the name of the property, I would refer to https://material-ui.com/guides/api/#property-naming

packages/grid/_modules_/grid/models/colDef/gridColDef.ts

packages/storybook/src/stories/grid-reorder.stories.tsx

packages/grid/x-grid/src/tests/reorder.XGrid.test.tsx

flaviendelangle · 2021-07-07T08:14:22Z

For the name of the property, I would refer to https://material-ui.com/guides/api/#property-naming

@oliviertassinari

This page is for props, I don't think the shorthand argument really applies here.
But I am 100% in favor of consistency.

Both approaches can make sense

Always favor default: false (meaning that we should have disableResize instead of resizable)
or
Always favor the affirmative form (meaning that we should have exportable instead of disableExport)

Most options are in the affirmative form so I think we should rename disableExport into exportable (my PR has not yet been merged) and on this PR orderable ?

flaviendelangle · 2021-07-07T08:55:22Z

@mui-org/x

I have two behavior points to discuss

AG Grid differentiate

the columns that can't be moved and that are automatically placed at the beginning of the grid (lockPosition: true)
the column that can't be moved but for which we can move columns around it (suppressMovable: true)

The Age column is locked as the first column in the scrollable area of the grid. It is not possible to move this column, or have other columns moved over it to impact its position. As a result the Age column marks the beginning of the scrollable area regardless of its position.
The Athlete column has moving suppressed. It is not possible to move this column, but it is possible to move other columns around it.

Do you think it's something we want to implement ?
I did a single solution with a bit more of magic since I'm checking the position of the target column to decide if the user is allowed to move the drag column on it.

Do we want to also block the re-ordering when it is done with apiRef.current.setColumnIndex ?
Right now I am only blocking the drag&drop.

oliviertassinari · 2021-07-07T09:43:10Z

Both approaches can make sense

@flaviendelangle Yeah, right, the shorthand advantage is specific to the props. I will update this section to mention the second advantage: as a developer, you always know what the default value is by looking at the name of the prop.

Do you think it's something we want to implement ?

I think that the difference is not negligeable. If we wanted to be really low-level and maximize flexibility, we could say to the developers:

suppressMovable: true: add event handlers on the column headers and use [RFC] Use event.defaultMuiPrevented to prevent default events #1403.
lockPosition: true it could be implemented with a controlled API and the prevent event.

I guess that for 2. to work, it also needs 1.

Maybe we can implement 2, and in the future, expose 1 in the API if we see a real use case? So far the problem we want to solve is to force the selection checkbox to stay on the starting position.

flaviendelangle · 2021-07-09T07:13:41Z

#2085 (comment) @oliviertassinari

What do you mean by "implemented with a controlled API" ?
For instance if we want to avoid dragging a column left of the checkbox column (or drag the checkbox column right of anything).

docs/src/pages/components/data-grid/columns/columns.md

packages/grid/_modules_/grid/models/colDef/gridColDef.ts

m4theushw · 2021-07-16T19:07:50Z

packages/grid/x-grid/src/tests/reorder.XGrid.test.tsx

+      const targetCol = getColumnHeaderCell(0).firstChild!;
+
+      fireEvent.dragStart(dragCol);
+      fireEvent.dragEnter(targetCol);


If the column can't be reordered we don't even allow the user to start dragging it. We could check if the draggable attribute is false or if GRID_COLUMN_HEADER_DRAGGING_CSS_CLASS was not added.

packages/grid/x-grid/src/tests/reorder.XGrid.test.tsx

m4theushw · 2021-07-16T19:41:58Z

packages/grid/_modules_/grid/hooks/features/columnReorder/useGridColumnReorder.tsx

+          !targetCol.disableReorder ||
+          (targetColVisibleIndex > 0 && targetColVisibleIndex < visibleColumnAmount - 1);
+
+        const cursorMoveDirectionX = getCursorMoveDirectionX(cursorPosition.current, coordinates);


For another PR:

This cursorMoveDirectionX is wrong. It's always "right" the first time. We need to save the cursor position when the user starts dragging

m4theushw · 2021-07-16T19:45:06Z

packages/grid/x-grid/src/tests/reorder.XGrid.test.tsx

+  const dragOverEvent = createEvent.dragOver(target);
+  // Safari 13 doesn't have DragEvent.
+  // RTL fallbacks to Event which doesn't allow to set these fields during initialization.
+  Object.defineProperty(dragOverEvent, 'clientX', { value: 1 });


For another PR:

Add an argument to allow to change the clientX. We use this value to check if the user is moving to the left or right.

packages/grid/_modules_/grid/hooks/features/columnReorder/useGridColumnReorder.tsx

oliviertassinari

I have tested the behavior with a disableReorder on a column in the middle: https://codesandbox.io/s/material-demo-forked-2c26p?file=/demo.js. It's weird. But 👍 for ignoring this for now. I think that it would be more efficient & effective to ship and wait for feedback.

I mean, the new prop allows preventing end-users to start the drag and drop from the column, it's already great. It comes with a bonus behavior (that is likely outside of the scope of disableReorder) of preventing other columns to go past it.

packages/grid/x-grid/src/tests/reorder.XGrid.test.tsx

m4theushw

Good job!

packages/grid/x-grid/src/tests/reorder.XGrid.test.tsx

flaviendelangle added 3 commits July 6, 2021 15:57

[DataGrid] Add ability to disable reorder on some columns

0ee8627

Write tests

527a8d2

Run prettier

a658b37

flaviendelangle changed the title ~~Disable reorder~~ [XGrid] Add ability to disable reorder on some columns Jul 6, 2021

flaviendelangle marked this pull request as draft July 6, 2021 14:38

Fix

8ebe40a

flaviendelangle self-assigned this Jul 6, 2021

oliviertassinari added components: XGrid new feature New feature or request labels Jul 6, 2021

oliviertassinari reviewed Jul 6, 2021

View reviewed changes

packages/grid/_modules_/grid/models/colDef/gridColDef.ts Outdated Show resolved Hide resolved

packages/storybook/src/stories/grid-reorder.stories.tsx Show resolved Hide resolved

packages/grid/x-grid/src/tests/reorder.XGrid.test.tsx Outdated Show resolved Hide resolved

oliviertassinari mentioned this pull request Jul 6, 2021

[core] Remove dead code #2088

Merged

Fix

bbdc088

Add doc

edd4ab0

flaviendelangle marked this pull request as ready for review July 7, 2021 13:47

Merge

058f56a

m4theushw mentioned this pull request Jul 16, 2021

[XGrid] How do I use columnHeaderDragStart and columnHeaderDragEnd #2158

Closed

m4theushw reviewed Jul 16, 2021

View reviewed changes

oliviertassinari reviewed Jul 17, 2021

View reviewed changes

packages/grid/x-grid/src/tests/reorder.XGrid.test.tsx Outdated Show resolved Hide resolved

flaviendelangle added 4 commits July 19, 2021 09:12

Merge branch 'master' into disable-reorder

b9a6fd6

Code review

3cf2636

Code review

330c18e

Fix

219b7e1

dtassone approved these changes Jul 19, 2021

View reviewed changes

Build doc

8f5e480

Fix

0887d9b

flaviendelangle requested a review from m4theushw July 20, 2021 07:36

Fix

313f41d

m4theushw approved these changes Jul 20, 2021

View reviewed changes

packages/grid/x-grid/src/tests/reorder.XGrid.test.tsx Outdated Show resolved Hide resolved

Code review

018a66b

flaviendelangle merged commit 7e93d3f into mui:master Jul 21, 2021

flaviendelangle deleted the disable-reorder branch July 21, 2021 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[XGrid] Add ability to disable reorder on some columns #2085

[XGrid] Add ability to disable reorder on some columns #2085

flaviendelangle commented Jul 6, 2021 •

edited by oliviertassinari

Loading

oliviertassinari commented Jul 6, 2021

flaviendelangle commented Jul 7, 2021

flaviendelangle commented Jul 7, 2021

oliviertassinari commented Jul 7, 2021 •

edited

Loading

flaviendelangle commented Jul 9, 2021 •

edited

Loading

m4theushw Jul 16, 2021

m4theushw Jul 16, 2021

m4theushw Jul 16, 2021

oliviertassinari left a comment •

edited

Loading

m4theushw left a comment

[XGrid] Add ability to disable reorder on some columns #2085

[XGrid] Add ability to disable reorder on some columns #2085

Conversation

flaviendelangle commented Jul 6, 2021 • edited by oliviertassinari Loading

oliviertassinari commented Jul 6, 2021

flaviendelangle commented Jul 7, 2021

flaviendelangle commented Jul 7, 2021

oliviertassinari commented Jul 7, 2021 • edited Loading

flaviendelangle commented Jul 9, 2021 • edited Loading

m4theushw Jul 16, 2021

Choose a reason for hiding this comment

m4theushw Jul 16, 2021

Choose a reason for hiding this comment

m4theushw Jul 16, 2021

Choose a reason for hiding this comment

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

m4theushw left a comment

Choose a reason for hiding this comment

flaviendelangle commented Jul 6, 2021 •

edited by oliviertassinari

Loading

oliviertassinari commented Jul 7, 2021 •

edited

Loading

flaviendelangle commented Jul 9, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading