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

[DataGridPremium] Server-side data source with row grouping #13826

Merged
merged 46 commits into from
Oct 25, 2024

Conversation

MBilalShafi
Copy link
Member

@MBilalShafi MBilalShafi commented Jul 15, 2024

πŸ‘¨β€πŸ’» Ready for Review πŸ‘¨β€πŸ’»

Part of #8179
Resolves #10859

Preview: https://deploy-preview-13826--material-ui-x.netlify.app/x/react-data-grid/server-side-data/row-grouping/

Action items

  • Add support for groupingValueGetter
  • Support row grouping features on useMockServer:
    • Sorting
    • Filtering
    • Pagination
    • Support more datasets
  • Rows with missing groups
  • Fix Argos' regression
  • Add/refine documentation

Follow-ups:

  • Support sort and filter on the Group column(s) for row grouping and tree data
  • Test coverage

@MBilalShafi MBilalShafi added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request plan: Premium Impact at least one Premium user feature: Row grouping Related to the data grid Row grouping feature feature: Server integration Better integration with backends, e.g. data source labels Jul 15, 2024
@tommy-wl
Copy link

Hi @MBilalShafi, are you able to provide an ETA for this? Thank you!

@MBilalShafi
Copy link
Member Author

Hey @tommy-wl, thank you for reaching out.

The ETA for this feature is around the end of Q3, its currently one of the main focus areas for the team. Feel free to test the under development feature and provide some early feedback if you want.

@MBilalShafi MBilalShafi changed the title [DataGridPremium] Server-side row grouping [DataGridPremium] Server-side data source with row grouping Jul 22, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 22, 2024

This comment was marked as outdated.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 4, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 16, 2024

This comment was marked as outdated.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 17, 2024
@MBilalShafi MBilalShafi added the needs cherry-pick The PR should be cherry-picked to master after merge label Oct 24, 2024
Comment on lines 57 to 59
if (value === undefined) {
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we should do this now, it's a breaking change (currently, undefined value is treated as false)

Copy link
Member Author

@MBilalShafi MBilalShafi Oct 24, 2024

Choose a reason for hiding this comment

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

I added it to avoid this:

image

For client-side row grouping we have a concept of autogenerated group nodes and we handle it like this:

if (params.field !== '__row_group_by_columns_group__' && isAutogeneratedRowNode(params.rowNode)) {
return '';
}

In server-side row grouping, all the rows are regular group rows and not marked as auto generated, so the above check doesn't work. Did a hacky'ish solution, let me know how does it feel.

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.

Solid feature! Great work!

@@ -252,6 +256,10 @@ Use the `setRowChildrenExpansion` method on `apiRef` to programmatically set the

{{"demo": "RowGroupingSetChildrenExpansion.js", "bg": "inline", "defaultCodeOpen": false}}

:::warning
The `apiRef.current.setRowChildrenExpansion` method is not compatible with the [server-side tree data](/x/react-data-grid/server-side-data/tree-data/) and [server-side row grouping](/x/react-data-grid/server-side-data/row-grouping/). Use `apiRef.current.unstable_dataSource.fetchRows` instead.
Copy link
Member

Choose a reason for hiding this comment

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

Curious, why it is not compatible? Could it call the data source to apply the behavior or is there a strong feasibility blocker?
(Question applies to both get and set methods)

Copy link
Member Author

Choose a reason for hiding this comment

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

Data source changes a bit how expansion works.
When the user expands a row, fetchRows is called, which fetches the rows internally and only on success it calls setRowChildrenExpansion internally which only updates the isExpanded state of the respective rowNode.

If the user tries to call setRowChildrenExpansion there will be no rows to show, only the expand icon will shift to expanded form, thus setRowChildrenExpansion is no longer recommended to use with the data source.

Copy link
Member

Choose a reason for hiding this comment

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

Good point @joserodolfofreitas
@MBilalShafi Couldn't we potentially update setRowChildrenExpansion to fetch rows if the data source is being used?

Copy link
Member

Choose a reason for hiding this comment

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

I missed the conversation here.
Should we create an issue to make these approaches compatible?

Copy link
Member Author

@MBilalShafi MBilalShafi Oct 29, 2024

Choose a reason for hiding this comment

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

Couldn't we potentially update setRowChildrenExpansion to fetch rows if the data source is being used?

@cherniavskii We certainly could. Mostly a DX decision. It hasn't been done in the current implementation because setRowChildrenExpansion sounds like (or anticipated as) a state setter (or a pure function), hiding the fetch part behind would be less aligned with a user's expectation as it will now have 3 possible outcomes.

  1. Set the state right away (no data source)
  2. Set the state after some time (waiting time of the BE API)
  3. Never set the state (fetching failed)

Using it from within fetchRows makes sense if it's understood to be only responsible for 1, or we may have to change its current perception.

What do you think?

@MBilalShafi MBilalShafi merged commit ae72b8c into mui:master Oct 25, 2024
15 of 16 checks passed
@MBilalShafi MBilalShafi deleted the datasource-row-grouping branch October 25, 2024 07:37
github-actions bot pushed a commit that referenced this pull request Oct 25, 2024
@arminmeh arminmeh mentioned this pull request Oct 25, 2024
@tommy-wl
Copy link

tommy-wl commented Oct 25, 2024

@MBilalShafi great work! Super excited to start using this feature as soon as it is released!

Quick question: just to confirm, does this work grouping multiple columns at the same time as well? https://mui.com/x/react-data-grid/row-grouping/#multiple-grouping-columns

Thanks

Premium customer - Support ID: 96081

@MBilalShafi
Copy link
Member Author

Hey @tommy-wl,

just to confirm, does this work grouping multiple columns at the same time as well? mui.com/x/react-data-grid/row-grouping#multiple-grouping-columns

Yes, single column and multiple column grouping both should work with this feature.

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! feature: Row grouping Related to the data grid Row grouping feature feature: Server integration Better integration with backends, e.g. data source needs cherry-pick The PR should be cherry-picked to master after merge new feature New feature or request plan: Premium Impact at least one Premium user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Implement server-side data source in row grouping
9 participants