-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[DataGridPro] Tree data row reordering #19401
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
base: master
Are you sure you want to change the base?
Conversation
Deploy preview: https://deploy-preview-19401--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
This comment was marked as outdated.
This comment was marked as outdated.
…prove auto expand logic
This comment was marked as outdated.
This comment was marked as outdated.
With row reordering, users can reorder tree data or move rows from one group to another. | ||
To enable this feature with row grouping, pass the `rowReordering` prop to the Data Grid component: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be changed to "pass the getTreeDataPath
and setTreeDataPath
props to the Data Grid component".
May be intro by mentioning the reordering feature first could help the reader understand that this is a feature on top of the reordering.
/> | ||
``` | ||
|
||
:::warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:::warning |
This warning can be removed. To me, those methods are required to make the feature work, it's not a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 👍!
I have checked the demos and they look awesome. Just some suggestions on the docs.
::: | ||
|
||
## Row reordering with tree data 🚧 | ||
## Disable reordering of specific rows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great additions to the row reordering feature 👏🏻
|
||
With this feature, users would be able to reorder rows in use cases that also involve tree data and/or row grouping. | ||
:::info | ||
The above demo uses row grouping to demonstrate the concept. You can check more about this in the [Row grouping—Drag-and-drop group reordering](/x/react-data-grid/row-grouping/#drag-and-drop-group-reordering) documentation section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better if the demo used Tree data, since it is the feature in the same plan as row reordering?
``` | ||
|
||
:::warning | ||
In order for the cross parent operations to work where there will be a change in the path, you need to pass the `setTreeDataPath()` prop, that works reverse to how [`getTreeDataPath()`](/x/api/data-grid/data-grid-pro/#data-grid-pro-prop-getTreeDataPath) works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order for the cross parent operations to work where there will be a change in the path, you need to pass the `setTreeDataPath()` prop, that works reverse to how [`getTreeDataPath()`](/x/api/data-grid/data-grid-pro/#data-grid-pro-prop-getTreeDataPath) works. | |
Row reordering can change the path of the row. | |
Pass the `setTreeDataPath()` prop to revert the operation done by [`getTreeDataPath()`](/x/api/data-grid/data-grid-pro/#data-grid-pro-prop-getTreeDataPath) while building the tree. |
|
||
{{"demo": "TreeDataReordering.js", "bg": "inline", "defaultCodeOpen": false}} | ||
|
||
### Sync row data with reordered data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Sync row data with reordered data | |
### Reordering controlled rows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I feel like this section should be called "Reordering persistance" instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that using the word "controlled" might cause confusion, as "rows" is a semi-controlled'ish prop which is only controlled one way (from outside to the Data Grid).
should be called "Reordering persistance" instead
Keeping in mind explanation in https://github.com/mui/mui-x/pull/19401/files#r2395786742 I think this is closer to the users' expectation. Will use it.
|
||
### Sync row data with reordered data | ||
|
||
If you want to update the external row data, for example to persist it in the local storage, you can use the `onRowOrderChange()` callback and the Data Grid selectors to get the reordered data and sync with the external data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to update the external row data, for example to persist it in the local storage, you can use the `onRowOrderChange()` callback and the Data Grid selectors to get the reordered data and sync with the external data. | |
If you want to update the external row data, for example, to persist it in the local storage, you can use the `onRowOrderChange()` callback and the Data Grid selectors to get the new row order and sync with the external data. |
If you want to update the external row data, for example to persist it in the local storage, you can use the `onRowOrderChange()` callback and the Data Grid selectors to get the reordered data and sync with the external data. | ||
|
||
```tsx | ||
<DataGridPro onRowOrderChange={handleRowOrderChange} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't persistance be implemented with the processRowUpdate
prop?
Same as with editing and row reordering with row grouping?
From what I see in the code, this is already implemented after setTreeDataPath
returns an updated row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't persistance be implemented with the processRowUpdate prop?
Technically, yes. However, processRowUpdate()
only handles cross-parent operations when actual row data is updated. It does not track reordering within the same parent. This limitation has been highlighted by users, for example, in this comment.
Ideally, we would have a controlled model like rowReorderModel / onRowReorderModelChange
. Since such an API does not currently exist, the Data Grid assumes that the row order matches the order of items provided in the rows prop.
This example demonstrates how to synchronize row order externally (for example, using component state or local storage). The approach is similar to the row ordering example in the docs, but adapted for the tree data case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for row reordering persistence to work with nested data, both processRowUpdate
and onRowOrderChange
should be used, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it depends on the use case.
Technically, onRowOrderChange()
alone could work, but it does not provide specific details about which rows were affected. This means users may need to perform a deep comparison with the previous rows if they want to persist specific row updates to a database. On the other hand, processRowUpdate()
is better suited for handling individual row updates (data), but it doesn't provide the same parent reorders or the drop position for cross-parent reorders.
As I understand it, persistence can be implemented in the following ways:
1. Cross-parent row updates only
If the relative order of rows within a parent does not matter, processRowUpdate()
alone is sufficient for persisting cross-parent row (data) updates.
2. Local or client-side state persistence
If row updates do not need to be saved to a database and only need to be synced with component state or local storage, using onRowOrderChange()
with the gridOrderedDataRowsSelector
is enough, since it provides a fresh ordered rows array that can be directly used with setState()
. Both order and data will be persisted this way. (The demo under discussion uses this approach)
3. Proper backend data persistence with order tracking
For more complex scenarios where both row data updates and relative row reorderings need to be persisted (for example, in a sequential database), a hybrid approach can be used. Here, processRowUpdate()
handles data updates while onRowOrderChange()
tracks reorder changes. I expect #8352 to improve this particular use-case.
Does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the details!
- Proper backend data persistence with order tracking
I think this is the scenario we should follow in our persistence demo.
Initially, I thought that the need to use both processRowUpdate
and onRowOrderChange
for persistence is weird. But on second thought, that's exactly what you'd do in case of a flat rows structure if you want to support Editing and Row reordering. The difference is that for nested rows, moving the row to a different parent === editing operation, so you have to use Editing API to persist such an action 👍🏻
In our docs we should explain that moving a row from one parent to another edits a row under the hood, and this is why the processRowUpdate
is necessary for persistance.
|
||
// Process row update if needed | ||
if (processRowUpdate) { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the same (or very similar) try...catch block being repeated multiple times, could you deduplicate it?
Also, why isn't BatchRowUpdater
used for all updates? Even when we know that only one row will be updated, it might still make sense if we let the updater handle the process. WDYT?
packages/x-data-grid-pro/src/hooks/features/treeData/treeDataReorderExecutor.ts
Show resolved
Hide resolved
class SameParentSwapOperation extends BaseReorderOperation { | ||
readonly operationType = 'same-parent-swap'; | ||
|
||
detectOperation(ctx: ReorderExecutionContext): ReorderOperation | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a chance to reuse the operation detection logic with row reordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean plain data reordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sorry, I meant grouped rows reordering
{ | ||
id: 20, | ||
path: ['Pictures', 'Family', 'Birthday.png'], | ||
name: 'Birthday.png', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider the possibility of letting developers control over the drop targets?
While playing with this example, I thought that if this was a real-world use case, you don't want to turn a file into a folder. You would want to be able to move files or folders between folders though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Developers should be able to use "isValidRowReorder()" to do that.
It seems to be a nice demo. I'll try to add it. 👍
display: 'none', | ||
}, | ||
}, | ||
[`& .${c['row--dropAbove']}`]: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious – what's the rationale for replacing this with GridRowDragAndDropOverlay
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Feasibility: The "over" usecase wasn't possible with just pseudo classes.
- Consistency: Tree view also uses a similar approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need these classes then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't. Nice catch, I forgot to clean them up. Will do.
Co-authored-by: Armin Mehinovic <[email protected]> Signed-off-by: Bilal Shafi <[email protected]>
Co-authored-by: Armin Mehinovic <[email protected]> Signed-off-by: Bilal Shafi <[email protected]>
Co-authored-by: Siriwat K <[email protected]> Signed-off-by: Bilal Shafi <[email protected]>
Co-authored-by: Armin Mehinovic <[email protected]> Signed-off-by: Bilal Shafi <[email protected]>
}, | ||
); | ||
|
||
export const gridOrderedDataRowsSelector = createSelector( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this is only used in the demo
Would it make sense to use GridRowOrderChangeParams
to manually create a new row order in the userland and not make a new selector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GridRowOrderChangeParams
in its current form isn't reliable, especially when mutiple rows are involved, e.g. when moving an entire group. I'm considering planning a breaking change in next major to let the users handle this with GridRowOrderChangeParams
by extending it's type.
Will add an item in v9 BCs with a proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could extend the type even now and add children
property to hold a part of the tree (for group nodes).
This should be enough, right?
Or there is something else that is not reliable, like stale data?
* @returns {void | Promise<void>} Returns a Promise when async operations are involved (e.g., processRowUpdate) | ||
*/ | ||
setRowIndex: (rowId: GridRowId, targetIndex: number) => void; | ||
setRowIndex: (rowId: GridRowId, targetIndex: number) => void | Promise<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change.
You also had to handle it in the grid row reorder hook
// Handle both sync and async cases
if (result && typeof result.then === 'function') {
await result;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need to wait for setRowIndex
to finish? If I understand correctly, the async
nature of setRowIndex
is coming from the fact that some operations involve persistence, but shouldn't that be in the background (i.e. we don't need to wait for it when calling setRowIndex
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to wait because in case of failure, the event (onRowOrderChange()
) should not be fired.
*/ | ||
onDataSourceError?: (error: GridGetRowsError<GridGetRowsParams> | GridUpdateRowError) => void; | ||
/** | ||
* Determines if a row is reorderable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Determines if a row is reorderable. | |
* Indicates if a row is reorderable. |
/** | ||
* Allows to disable certain row reorder operations based on the context. | ||
* The internal validation is still applied which allows maximum supported use-cases. | ||
* Use `isValidRowReorder()` to omit some of the default validation rules. | ||
* @param {ReorderValidationContext} context The context object containing all information about the reorder operation. | ||
* @returns {boolean} A boolean indicating if the reorder operation should go through. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* Allows to disable certain row reorder operations based on the context. | |
* The internal validation is still applied which allows maximum supported use-cases. | |
* Use `isValidRowReorder()` to omit some of the default validation rules. | |
* @param {ReorderValidationContext} context The context object containing all information about the reorder operation. | |
* @returns {boolean} A boolean indicating if the reorder operation should go through. | |
*/ | |
/** | |
* Indicates if a row reorder attempt is valid. | |
* Can be used to disable certain row reorder operations based on the context. | |
* The internal validation is still applied, preventing unsupported use-cases. | |
* Use `isValidRowReorder()` to add more validation rules to the default ones. | |
* @param {ReorderValidationContext} context The context object containing all information about the reorder operation. | |
* @returns {boolean} A boolean indicating if the reorder operation should go through. | |
*/ |
This adds rules, you can't omit the default, right?
|
||
let apiRef: RefObject<GridApi | null>; | ||
|
||
// Suppress `act()` warnings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
/** | ||
* The current drop target information. | ||
*/ | ||
dropTarget: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this in the state
can't we just listen to mouseOver
on the row?
dragDirection: DragDirection; | ||
targetRowIndex: number; | ||
sourceRowIndex: number; | ||
expandedSortedRowIndexLookup: Record<GridRowId, number>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be fetched with the selector, so we don't have to pass it around.
targetRowIndex
and sourceRowIndex
are in a similar situation.
If you need them, you can get them from targetNode
and sourceNode
and the selector
return index; | ||
}; | ||
|
||
const createTreeData = (): GridRowsProp => [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just
const createTreeData = (): GridRowsProp => [ | |
const treeData: GridRowsProp = [ |
const leafAIndex = allValues.indexOf('LeafA'); | ||
const leafBIndex = allValues.indexOf('LeafB'); | ||
|
||
// If not found by name, look for node IDs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems unnecessary
is the indexOf
sometimes missing to find the leaf?
expect(handleRowOrderChange.callCount).to.equal(0); | ||
}); | ||
|
||
it('should render correctly with isRowReorderable prop', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: could be part of the previous test, where you try dragging other row after the first one was ignored
const documentsIndex = allValues.indexOf('Documents'); | ||
const workIndex = allValues.indexOf('Work'); | ||
|
||
if (documentsIndex >= 0 && workIndex >= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we have an error if we can't find the rows instead of skipping the assertion?
There are a couple more if
statements that do the same
const result = apiRef.current.setRowIndex(dragRowId, validatedIndex); | ||
|
||
// Handle both sync and async cases | ||
if (result && typeof result.then === 'function') { | ||
await result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's necessary (see #19401 (comment)), but you can await function regardless whether it's sync or async:
const result = apiRef.current.setRowIndex(dragRowId, validatedIndex); | |
// Handle both sync and async cases | |
if (result && typeof result.then === 'function') { | |
await result; | |
} | |
await apiRef.current.setRowIndex(dragRowId, validatedIndex); |
// Execute callback and wait if it's async | ||
const result = callback(); | ||
if (result && typeof result.then === 'function') { | ||
await result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Execute callback and wait if it's async | |
const result = callback(); | |
if (result && typeof result.then === 'function') { | |
await result; | |
} | |
await callback(); |
name: 'over-position', | ||
applies: (ctx) => ctx.dropPosition === 'over', | ||
isInvalid: (ctx) => ctx.targetNode.type !== 'leaf', | ||
message: 'Cannot drop over a non leaf node', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine if it stays like this
But, we can also allow this and consider it as a drop to the last position in the group
It would be like dropping a file in the folder without opening that folder
isInvalid: (ctx) => { | ||
// check if the target is a descendent of the source | ||
// 1. Direct child case | ||
if (ctx.targetNode.parent === ctx.sourceNode.id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already covered by the loop below, so it can be removed
message: 'Cannot drop below collapsed group', | ||
}, | ||
{ | ||
name: 'drop-on-leaf-descendant-check', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already covered by 'group-to-leaf'
for any drop position?
Resolves #7774
https://deploy-preview-19401--material-ui-x.netlify.app/x/react-data-grid/tree-data/#drag-and-drop-tree-data-reordering
Checklist:
setTreeDataPath()
to work alongsidegetTreeDataPath()
isRowReorderable()
to stop some rows from being reordered at allisValidRowReorder()
to allow users to fine-tune the complex reorder operationsrowOrderChange
eventdataSource
counterpart: [data grid] Add row reordering support for server side grouped data #18947