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

[core] Implement tree-based row management #2813

Closed
flaviendelangle opened this issue Oct 9, 2021 · 4 comments · Fixed by #2996
Closed

[core] Implement tree-based row management #2813

flaviendelangle opened this issue Oct 9, 2021 · 4 comments · Fixed by #2996
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes

Comments

@flaviendelangle
Copy link
Member

flaviendelangle commented Oct 9, 2021

Part of #212 and #210

The code will be extracted from #2725 once #2673 and #2782 are merged.

This issue is a breaking change and should therefore be merged before the release of the v5 stable version.
The rest of the Tree Data and Row Grouping by Column should not be breaking and can be finished after the stable version release.


Goal

Have the useGridRows / useGridSorting and useGridFilter work with tree structure to be compatible with features where rows can have children (#212, #210 and somehow #211).

When the rows have children, the sorting / filtering must be applied on the top level rows but also on the children of each depth (except with props.disableChildrenFiltering and props.disableChildrenSorting)
This new structures allows us to have the same code to apply the sorting and filtering whether or not some rows have children.

API

To new methods to interact with the tree structure

export interface GridRowApi {
  UNSTABLE_getRowNode: (id: GridRowId) => GridRowConfigTreeNode | null;
  UNSTABLE_setRowExpansion: (id: GridRowId, isExpanded: boolean) => void;
}

Sorting

useGridSorting must apply the sorting to each depth of the tree (except if props.disableChildrenSorting === true)

For instance :

// Before sorting
C
A
  A.B
  A.A
B
  B.A
  B.B

// After sorting
A
  A.A
  A.B
B
  B.A
  B.B
C

Filtering

useGridFilter must apply the sorting to each depth of the tree (except if props.disableChildrenFiltering === true)

A node is now visible if one of the following conditions is filled:

  • One of its children is passing the filter
  • He is passing the filter
@flaviendelangle flaviendelangle added status: waiting for maintainer These issues haven't been looked at yet by a maintainer breaking change core Infrastructure work going on behind the scenes and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 9, 2021
@flaviendelangle flaviendelangle added this to the v5 stable version milestone Oct 9, 2021
@flaviendelangle flaviendelangle self-assigned this Oct 9, 2021
@flaviendelangle flaviendelangle added the component: data grid This is the name of the generic UI component, not the React module! label Oct 9, 2021
@m4theushw
Copy link
Member

Did you consider keeping the rows as a list and flattening the tree? This library implements a tree view using this method.

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Oct 11, 2021

In both the tree data and the row grouping, the prop.rows is still a list.
The tree is only an internal representation to help applying the sorting and interacting with the tree (toggle expansion for instance).

There are two places where I store as a tree:

  • state.rows.tree
  • state.rows.sorting.sortedRows

We could probably keep those structures flat with a pointer system (like here https://github.com/dowjones/react-dropdown-tree-select/blob/develop/src/tree-manager/flatten-tree.js)
I can take a deeper look at the consequences.

I fear that for the sorting it will make the implementation more complex. Right now the sorting just walk the tree and sort every set of child as it was doing before on the full list.

And for the tree interactions like the toggling of a row expansion, both would probably be quite similar.


For now, the decision is not blocking to review the PRs related to the tree data
In #2840, we would only need to modify the return type of the methods but the logic remains the same.
I'll do the flat implementation and look at what are the gains and the losses on a real full implementation.


EDIT: I reverted the changes for apiRef.current.getRowModels
This method is never used internally and the breaking change did not make enough sense 👍

@flaviendelangle
Copy link
Member Author

@m4theushw The PRs preparing for the new virtualization totally broke my ugly hack for the Tree Data so I can't test the rendering part :D

But I implemented the flat structure and for the sorting / filtering it seems to work fine.
The sorting is a little more complex, but for the filtering it allows to almost change nothing which is nice.

@flaviendelangle flaviendelangle changed the title [core] Store the rows in state in a tree structure instead of a list [core] Implement tree-based row management Oct 22, 2021
@flaviendelangle
Copy link
Member Author

I spent some time looking at AG-Grid implementation, especially the partial data fetching (fetching the children of a row when expanding it for instance). And I think it will require to go back to a real tree at some point.

But the modifications will be very deep to do it properly (for instance do not reapply the whole sorting when we update a list of children, as it does not impact the rest of the tree).
So it will be in v6+ anyway.

For the 1st version of #212 and #210, the flat structure limit a lot the breaking changes so I prefer to go this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants