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] Allow to automatically group the rows in a Tree Data #2725

Merged
merged 614 commits into from
Nov 24, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Sep 28, 2021

Closes #210 (I will create new issues for the following features in the Tree Data)

Preview doc

Merging plan

  1. Merge [core] Each hook should initialize its state synchronously #2782

  2. Extract independent features in stand-alone PRs

  1. Extract the selector breaking changes [DataGrid] Unify filtering / sorting / pagination / rows selectors #2942

  2. Extract the list => tree migration in useGridRows, useGridFilter, useGridSorting and rendering [core] Implement tree-based row management #2996

  3. Extract the data generator updates [core] Adapt useDemoData for Tree Data #2978

  4. Merge the useGridTreeData and directly related features + its documentation

1-3 should be done before the major release because 3. is a breaking change.

@flaviendelangle flaviendelangle self-assigned this Sep 28, 2021
@flaviendelangle flaviendelangle changed the title [DataGridPro] Tree data [DataGridPro] Tree data (WIP) Sep 28, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 1, 2021
@github-actions
Copy link

github-actions bot commented Oct 1, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 4, 2021
@github-actions
Copy link

github-actions bot commented Oct 5, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 5, 2021
@github-actions
Copy link

github-actions bot commented Oct 5, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 6, 2021
@github-actions
Copy link

github-actions bot commented Oct 7, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 7, 2021
@github-actions
Copy link

github-actions bot commented Oct 8, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 12, 2021
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 13, 2021
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@m4theushw
Copy link
Member

@flaviendelangle
Copy link
Member Author

@m4theushw Done 👍

@sanjinbecirovic
Copy link

Is there an ETA when this feature will be released?

@flaviendelangle
Copy link
Member Author

@sanjinbecirovic we don't have a fixed date but depending on the feedback it could be for the 02/11.
We prefer to review thoroughly this one to avoid bad surprises and breaking changes to fix bugs / unintended behaviors 👍

@alexfauquette
Copy link
Member

Small design detail, the arrow icon should be in the other direction when children are open. See the "Thomas" row in the DataGrid and the "Component API" in the doc menu

image
image

@m4theushw
Copy link
Member

Small design detail, the arrow icon should be in the other direction when children are open. See the "Thomas" row in the DataGrid and the "Component API" in the doc menu

Is not the sidebar that is wrong? In most of the demos in https://mui.com/components/accordion/#customization, the arrow is pointing up when the item is open. Only https://mui.com/components/accordion/#customization reproduces the same pattern as the sidebar. cc @danilo-leal

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

The data generator is generating duplicated paths. This causes unwanted behaviors if users open the demo in CodeSandbox and change the number of rows to something big, like 2k rows. See this example: https://codesandbox.io/s/treedatafullexample-material-demo-forked-hkm7q?file=/demo.tsx

image

I think we could also send an error to the console if the paths are not unique, similar to what happens with the key prop. It took me some time to figure out they weren't unique.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 22, 2021
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 23, 2021
@flaviendelangle
Copy link
Member Author

@m4theushw concerning the non-uniq paths, I indeed only applied the logic to the non-seeded generator, so below 1000 lines.
I will add an error when using the generator with tree data and more than 1000 lines for now 👍

For the unicity test, same thing I think it can be done in a folow up PR. And probably in dev mode only to avoid overhead

onChange={(event) => setDisableChildrenSorting(event.target.checked)}
/>
}
label="Enable `disableChildrenSorting`"
Copy link
Member

Choose a reason for hiding this comment

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

> For instance:

```ts
// The row A.A is immediately after its parent
Copy link
Member

Choose a reason for hiding this comment

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

The code should be inside the warning.

const validRows = [{ path: ['A'] }, { path: ['A', 'A'] }, { path: ['B'] }];

// The row A.A is not immediately after its parent
const invalidRows = [{ path: ['A'] }, { path: ['B'] }, { path: ['A', 'A'] }];
Copy link
Member

@m4theushw m4theushw Nov 23, 2021

Choose a reason for hiding this comment

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

We could add emojis to make clear what is the wrong pattern. Two examples: https://mui.com/guides/minimizing-bundle-size/#option-1 and https://mui.com/customization/how-to-customize/#main-content

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

Great work! I don't have anything to add. 💯

My only concern is that the code for sorting and filtering tree data nodes shouldn't be shipped as MIT, however, this can be addressed later.

Merging with master should fix the screenshot below:

image

flaviendelangle and others added 2 commits November 24, 2021 14:01
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
@sanjinbecirovic
Copy link

@sanjinbecirovic we don't have a fixed date but depending on the feedback it could be for the 02/11. We prefer to review thoroughly this one to avoid bad surprises and breaking changes to fix bugs / unintended behaviors 👍

Thank you for the reply, 02/11 is a release or? I've seen the code has been merged? but not released?

@flaviendelangle
Copy link
Member Author

02/12 not 02/11 my bad.
Yes it should be release on the 02/12, or in the days just after if the release takes more time to prepare.
In any case, it will be in the next release and we have a release planned tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: important This change can make a difference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] Implement Tree data
4 participants