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

[DataGrid] Add columnGroupingModel #5133

Merged
merged 56 commits into from
Aug 23, 2022
Merged

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Jun 7, 2022

Fix #195

  • Add columnGroupingModel to manage the tree of column groups
  • Compute header height according to the max depth of the column groups tree
  • Support column resize
  • Render the column group
  • Improve accessibility with role="columnheader" and correct column spanning
  • Support virtualization engine when only some columns of the group are rendered
  • Limit column drag&drop to the same column group
  • Add doc page
  • Add test
    • convert model to component
    • throw error for inconsistency in the model (struggling)
    • reorder behavior
  • Mark it as an experimental feature

docs preview: https://deploy-preview-5133--material-ui-x.netlify.app/x/react-data-grid/column-groups/

with default style: https://codesandbox.io/s/basicgroupingdemo-demo-mui-x-forked-6vhr5j?file=/demo.tsx
with show right border style (it seems buggy when the total width is smaller than the container width): https://codesandbox.io/s/basicgroupingdemo-demo-mui-x-forked-7si9wc?file=/demo.tsx
performance demo: https://codesandbox.io/s/basicgroupingdemo-demo-mui-x-forked-fj4qcs?file=/demo.tsx

Changelog

You can now group columns by using the columnGroupingModel prop. This allows you to display more structured data.

image

To enable this feature, add experimentalFeatures={{ columnGrouping: true }}.
The grouping header can be fully customized.
See the documentation to explore everything it has to offer.

@mui-bot
Copy link

mui-bot commented Jun 7, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 320.2 981.7 537.4 549.98 242.193
Sort 100k rows ms 483.9 1,133.8 485.1 755.86 248.356
Select 100k rows ms 207.5 295.6 247.5 250.66 33.515
Deselect 100k rows ms 130.4 252.9 177.7 193.14 45.547

Generated by 🚫 dangerJS against e6e383e

@alexfauquette alexfauquette force-pushed the columnGroup branch 2 times, most recently from 51ff034 to ee623e4 Compare June 10, 2022 12:16
@alexfauquette
Copy link
Member Author

About vitualization:

Before/after the first/last rendered column, I loop on other visible as long as they extend the first/last group of a given header row. This allows to render groups that always have the correct width:

image
image

Virtualization can be tested here:
https://codesandbox.io/s/mui-material-ui-forked-mtqtx9?file=/src/App.tsx

100 columns grouped according to their binary expression

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 10, 2022
@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 Jun 14, 2022
@gerdadesign
Copy link
Member

After speaking with @joserodolfofreitas, how do we feel about something along these lines to show column grouping (grouping by text size + border-bottom)? This would require having the column separator appear on hover to avoid feeling too overcrowded and busy.

Separator on hover — header row

DataGrid-seperatorOnHover-HeaderRow

Separator on hover — column group row 1

DataGrid-seperatorOnHover-HeaderGroupRow1

Separator on hover — column group row 1

DataGrid-seperatorOnHover-HeaderGroupRow2

Figma exploration for reference

@alexfauquette
Copy link
Member Author

@gerdadesign About the font-size varying depending on the hierarchical level, I'm a bit concerned because for now, we use only one font-size which is fixed for all the data grid as being theme.typography.body2

I've updated the codesandbox such that you can experiment with the different density factors

@alexfauquette
Copy link
Member Author

@m4theushw I was looking at the aria-rowcount property, and it seems that every row should be counted. INcluding the column header (and now the rows of the column grouping)

I was not the case before:

  • header did not have aria-rowindex
  • aria-row-count was set to the number of rows and not the number of rows+1 (+1 for the header)

Is their a specific reason for that or can I modify all the tests to let them match the new behavior (include header in the rowcount)?

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 so far. I think that columns not belonging to a named column group should belong to an empty group, so the column headers styling is consistent. It appears to be how AG Grid works: https://plnkr.co/edit/8TQhvj4m6obGYy2T?open=main.js

>
{depthInfo.elements.map(({ groupId, width, fields, colIndex }, groupIndex) => {
return (
<GridColumnGroupHeader
Copy link
Member

Choose a reason for hiding this comment

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

Could GridColumnHeaderItem be reused to have the same styling? We could add props to make it not clickable.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible but it will be a bit hacky, because GridColumnHeaderIteminfers a lot of properties from column so I will have to provide a column props faking the column state from the parameters of the grouping model.

I'm also concerned by the number of conditions to introduce in GridColumnHeaderItem to support the two types of headers.


For example data-field becomes data-fields to be able to have a smooth resizing

The resizing function of a group will probably be different from the resizing of a column because for a column on each mouseMove we can get the difference before/after and apply the 2px modification to all cells.
But when a column group containing 4 columns is resized by 1px I'm not sure we can modify all rows by 0.25px

When introducing a collapsed/extended cell it will be new action buttons to set


Could it be possible to keep GridColumnHeaderItem and GridColumnGroupHeaderItem with their own interfaces and internal logic, but reuse the same styled-components in both such that we kept a unified style, but have custom props/logic adapted for each case

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried something by introducing <GridGenericColumnHeader /> the two components GridColumnHeaderItem and GridColumnGroupHeader are now kind of wrapper, that adapt their props to the GridGenericColumnHeader

Copy link
Member

Choose a reason for hiding this comment

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

I think the current approach is good. I'm most concerned about reusing the same class names.

@m4theushw
Copy link
Member

header did not have aria-rowindex

I see ariw-rowindex in the column headers row. I checked https://mui.com/x/react-data-grid/

aria-row-count was set to the number of rows and not the number of rows+1 (+1 for the header)

Yeah, according to the example in https://w3c.github.io/aria-practices/examples/grid/dataGrids.html#ex3_label the row count should include the headers. I don't know why this was left.

@gerdadesign
Copy link
Member

In the demo, the information still seems clear without the additional type sizes, so perhaps this is ok to keep constant. I am seeing some differentiation in text weight and border thickness & color, though — can we verify it's the same as the other borders?
Screen Shot 2022-06-22 at 12 36 26 PM
Screen Shot 2022-06-22 at 12 40 55 PM

@alexfauquette
Copy link
Member Author

@m4theushw About putting columns that do not belong to a group in the grouping model I do not understand what is the usecase.

For AG grid, I understand, because grouping is defined in the columnDefs so columns are here because you have to define them. For now, I defined the column grouping as a model distinct from the columns props.

So if a column does not appear in the grouping, by default it has no group

@m4theushw
Copy link
Member

About putting columns that do not belong to a group in the grouping model I do not understand what is the usecase.

It won't have any functionality, it's more if users want to always show the bottom border, then have like a vertical separator to separate the groups. The way it was done here the border is in the groups. This can also be achieved with CSS.

@github-actions
Copy link

github-actions bot commented Jul 6, 2022

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

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

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Apart from few minor suggestions, looks good to me 👍

@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 the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 13, 2022
isResizing={false}
sortDirection={null}
hasFocus={false}
tabIndex={0}
Copy link
Member

Choose a reason for hiding this comment

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

The column group headers don't have keyboard navigation with the arrow keys, but with tabIndex=0 we can navigate with Tab between them. This is breaks the idea of only-one-focusable-element-per-grid from https://www.w3.org/WAI/ARIA/apg/patterns/grid/. I think they should be non-interactive.

Suggested change
tabIndex={0}
tabIndex={-1}

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 16, 2022
@gerdadesign
Copy link
Member

Was there a decision made for the default group header size? If we plan to support filtering, sorting, and other actions available in the column menu, should we consider match the header height?

@alexfauquette
Copy link
Member Author

@gerdadesign With the current space, seems that buttons can fit in the smaller height

image

In "compact" mode the ripple overflows a bit

image

The choice is between:

  • Smaller height to get group name closer to the column header
  • Same height to match what most of the other libraries are doing

About the menu, I do not think it will be as important as the column menu. In a group, filtering and sorting is not intuitive. The only action I found is group expansion which allows hiding/showing multiple columns on a single click, here is the AG-Grid example about sport results

@alexfauquette alexfauquette merged commit 81bc636 into mui:master Aug 23, 2022
alexfauquette added a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
Co-authored-by: Flavien DELANGLE <flaviendelangle@gmail.com>
Co-authored-by: Danail Hadjiatanasov <hadjiatanasov@gmail.com>
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Co-authored-by: José Rodolfo Freitas <joserodolfo.freitas@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 28, 2022

@alexfauquette
Copy link
Member Author

Just checked with @joserodolfofreitas we can mark them as done. I'm opening the PR

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 9, 2022

@alexfauquette Awesome thanks, I linked the other PRs to this one so it's easier to understand the history.

@mui/xgrid On different notes, I landed on this page https://next.mui.com/x/react-data-grid/column-groups/ and noticed a few things, maybe for future follow-ups:

  1. API design. Since the feature can be enabled with a single prop, it seems that columnGroupingModel would have been better named unstable_columnGroupingModel. This way, we can drop the experimentalFeatures, allowing developers to more easily follow BCs. I'm mostly raising this for future cases. IMHO experimentalFeatures should be limited to cases where we don't have any other API to enable a feature that isn't production ready.
  2. The demo in https://next.mui.com/x/react-data-grid/column-groups/#define-column-groups could benefit from having a single-column group (instead of two). I would imagine that in most cases, in group is enough. it would make the demo easier to follow, and the demo feels better. It would
  3. I struggle with the current look & feel, something doesn't fly for me, but I can't figure out how to really solve this. @gerdadesign I have tried:
Frame 1 (1)

vs. https://mui.com/x/react-data-grid/column-groups/#BasicGroupingDemo.tsx, more inlined with https://mui.com/material-ui/react-table/#column-grouping, it feels better, but it's not awesome either.
https://www.infragistics.com/products/ignite-ui-angular/angular/components/grid/collapsible-column-groups#angular-grid-collapsible-column-groups-overview-example is an interesting take but doesn't feel soo much MD.

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! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Implement Column groups
10 participants