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] Add doc example for tree data children lazy loading #3657

Merged
merged 4 commits into from Jan 18, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jan 18, 2022

Part of #3377
Doc preview

@flaviendelangle flaviendelangle added docs Improvements or additions to the documentation component: DataGridPro labels Jan 18, 2022
@flaviendelangle flaviendelangle self-assigned this Jan 18, 2022
@mui-bot
Copy link

mui-bot commented Jan 18, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 146.2 319 251.9 228.06 67.234
Sort 100k rows ms 315.2 544.8 508.2 477.9 82.628
Select 100k rows ms 143.9 284.3 179.8 191.46 50.344
Deselect 100k rows ms 94.4 221.3 221.3 147.12 49.106

Generated by 🚫 dangerJS against 8ee0cb0

Copy link
Member

@joserodolfofreitas joserodolfofreitas left a comment

Choose a reason for hiding this comment

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

Looking good 👍
Just a small suggestion.

Co-authored-by: José Rodolfo Freitas <joserodolfo.freitas@gmail.com>
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Looks good.

You could add one or two sentences before the demo to say:

  • we add a property descendantCount to rows, to know which group cell can be extended
  • we override both <GouppingCell /> and handleRowExpansionChange to take this new property into account

I should simplify the reading, but I'm not sure it is needed

Comment on lines 160 to 161

const GroupingCellWithLazyLoading = (props) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is already a complex component. Could add a comment to say that it is not the main one, and explain briefly what it does. For example

This component override the Grouping cell to show the expand/collapse icon according to the descendantCount property instead of the descendants property

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 👍

React.useEffect(() => {
fakeDataFetcher().then(setRows);

const handleRowExpansionChange = async (node) => {
Copy link
Member

Choose a reason for hiding this comment

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

By curiosity, why putting it in a useEffect instead of a useCallback ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This useEffect is only called once to initialize the event listeners and init the top level rows.

@flaviendelangle flaviendelangle merged commit 0733a43 into mui:master Jan 18, 2022
@flaviendelangle flaviendelangle deleted the tree-data-lazy-loading branch January 18, 2022 17:07
siriwatknp pushed a commit to siriwatknp/material-ui-x that referenced this pull request Jan 20, 2022
siriwatknp pushed a commit to siriwatknp/material-ui-x that referenced this pull request Jan 20, 2022
siriwatknp pushed a commit to siriwatknp/material-ui-x that referenced this pull request Jan 20, 2022
siriwatknp pushed a commit to siriwatknp/material-ui-x that referenced this pull request Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants