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

[data grid] Implement Aggregation #213

Closed
oliviertassinari opened this issue Aug 22, 2020 · 28 comments · Fixed by #4208
Closed

[data grid] Implement Aggregation #213

oliviertassinari opened this issue Aug 22, 2020 · 28 comments · Fixed by #4208
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Aggregation Related to the data grid Aggregation feature linked in docs The issue is linked in the docs, so completely skew the upvotes new feature New feature or request plan: Premium Impact at least one Premium user

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 22, 2020

Benchmark

@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Aug 22, 2020
@oliviertassinari oliviertassinari added the linked in docs The issue is linked in the docs, so completely skew the upvotes label Nov 30, 2020
@oliviertassinari oliviertassinari changed the title [DataGrid] Implement Aggregation [XGrid] Implement Aggregation Jul 7, 2021
@oliviertassinari oliviertassinari added components: XGrid and removed component: data grid This is the name of the generic UI component, not the React module! labels Jul 7, 2021
@oliviertassinari oliviertassinari changed the title [XGrid] Implement Aggregation [DataGridPro] Implement Aggregation Aug 30, 2021
@oliviertassinari oliviertassinari added plan: Pro Impact at least one Pro user plan: Premium Impact at least one Premium user labels Dec 5, 2021
@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! and removed plan: Pro Impact at least one Pro user labels Jan 29, 2022
@camnealie
Copy link

loving grouping (far out yall are killing it with features), really looking forward to aggregation!

@oliviertassinari oliviertassinari changed the title [DataGridPro] Implement Aggregation [data grid] Implement Aggregation Mar 16, 2022
@flaviendelangle flaviendelangle self-assigned this Mar 22, 2022
@flaviendelangle
Copy link
Member

flaviendelangle commented May 18, 2022

UI decision for the label of the footer aggregation

Other libraries

Telerik

The label of the aggregation is rendered on each column

Advantages:

  • Works great with any amount of aggregation

Disadvantages:

  • Needs large columns

image

AG-Grid

On footer, we can only aggregate with sum, so we can put the label on the 1st column

image

Solutions for our project

Side note: By "1st column", I mean a column where we decide to put the label of the footer. When row grouping is being used, it will be the on the grouping column(s), when the tree is flat, the user has to provide an explicit column field.

Right now on #4208, I have implemented the solution A which seems great when only one column is being used, but does not scale well if a user is aggregating with "max" on a column and "min" on another for instance.

For each couple of image below, the 1st one is "gross: max, year: max" and the 2nd one is "gross: max, year: min"

image

image

Solution A: Always on 1st column

Render the label on the 1st column if all columns are using the same aggregation function
Use "Aggregation" label on the 1st column otherwise

image

image

Advantages

  • Very clean when only one aggregation function is being used
  • Always the same behavior
  • No layout impact on columns except the 1st one

Disadvantages

  • Not very explicit when several aggregation function are being used (the user can override the "Aggregation" label but still he won't be able to have a very clean behavior

Solution B: Always on data column

Advantages

  • Always the same behavior
  • No layout change between single and multi function aggregation

Disadvantages

  • Impact the layout of the data column (the dev needs to put large columns to be able to fit the label)
  • For simple use cases with only one aggregation function, having the label on the 1st column feels cleaner to me

image

image

Solution C: On 1st column if 1 aggregation function, on data column if several

Advantages

  • Very clean when only one aggregation function is being used
  • Always explicit

Disadvantages

  • Two different behaviors depending on the amount of aggregation functions being used
  • Impact the layout of the data column (the dev needs to put large columns to be able to fit the label

image

image

@flaviendelangle
Copy link
Member

@mui/x if you can have a look when you have some time

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented May 20, 2022

Great work summarizing the solutions, @flaviendelangle!

I think Solution A alone is very limited, and the distance between the label and the aggregated value is potentially a usability issue.

I'm up for Solution B as the default behavior.
But it'd be great to have one option for customization: aggregationFooterLabel - to add a general label to the footer, so the user can get Solution A if it's desired. When the user sets this prop, we don't show the labels per column. (there are some use cases not covered, like when the user wants both the labels per column and the one on the first column, but we can leave that enhancement for upvoting)

Regarding the layout impact of Solution B, is it not feasible to use column spanning (when available) to extend the footer column without impacting the whole grid?

@flaviendelangle
Copy link
Member

Regarding the layout impact of Solution B, is it not feasible to use column spanning (when available) to extend the footer column without impacting the whole grid?

With which column would you span ?
The column on the left and right of the aggregated column could be aggregated as well.
And if we span left if the right column is not aggregated, I think it would create a weird behavior where the label moves depending on the status of the right column (same for left spanning).

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented May 20, 2022

With which column would you span ?

If feasible, I was thinking, as a basic rule of thumb, to always span to the left if the left column is available.
Feels like it'd cover most cases.
I think the occasional move of the label is ok since it'd move as a result/feedback when the user decides to use the "spanned" column to aggregate a new value.

@flaviendelangle
Copy link
Member

flaviendelangle commented May 20, 2022

From a non technical persipective:
It depends a lot on how many columns we think people will aggregate in average.
On AG-Grid, we have example with almost every column is aggregated (https://www.ag-grid.com/javascript-data-grid/grouping-footers/#example-customising-footer-values), and it would be weird to have

image

Or even something like this (no idea how we could align the value correctly by the way)

image

Additionally, how would it behave when there is also column spanning created by the user ?


I am honestly not a huge fan of playing with row spanning for this.
Depending on the width of your columns, the behavior will be weird.
Your left column could be 400px large, the label would be very far from the value of the rows it's aggregating.

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented May 20, 2022

You're right. Better to deal with the layout impact.

edit: How about customizing the footer? Could the dev setup a custom footer with col spanning?

@flaviendelangle
Copy link
Member

flaviendelangle commented May 25, 2022

edit: How about customizing the footer? Could the dev setup a custom footer with col spanning?

He has access to the footer ID, so I guess he can use column spanning manually if he wants.

But what would be the default behavior ? One of my 3 solutions or a 4th one ?

@cherniavskii
Copy link
Member

My personal preference is option C

@flaviendelangle
Copy link
Member

Solution D: Always on the data column header

image

Advantages

  • Always the same behavior
  • No layout change between single and multi function aggregation

Disadvantages

  • Impact the layout of the data column (the dev needs to put large columns to be able to fit the label or we would need to tweak the column width internally which is probably worse)

@alexfauquette
Copy link
Member

alexfauquette commented May 30, 2022

What about a prop footerAggregationFormatter = (columnField, aggregationModel, visibleColumns) => (agregatedValue) => string. From the model and the visible columns, you generate a formatted that will be responsible for adding or not the maximum: ... in the footer

This would allow switching easily between solutions B and C by exporting a function for each solution.

Otherwise, solution C seems to be the more flexible solution

@m4theushw
Copy link
Member

Based on https://experience.sap.com/fiori-design-web/analytical-table-alv/#aggregate I think we shouldn't add a default label on any column or header. IMO, displaying the aggregate value in bold is enough to discover that it means when using the most common aggregation functions (sum or count). Showing the value sometimes inside the column but other times in the first column may surprise users. This doesn't block us from adding a prop to customize the last row and also a demo showing how to do it. Although, if we do want to show a label, I would be in favor of an "i" icon, with a tooltip, to the left of each aggregate value, so option B+.

@flaviendelangle
Copy link
Member

flaviendelangle commented May 31, 2022

For me the goal are

  1. A clean behavior out of the box when using a single aggregation function
  2. A decent behavior out of the box when using several aggregation functions
  3. The ability to easily customize to have a clean behavior when using several aggregation functions (for instance users should not have to subscribe to events to customize it)
  4. As little layout shift as possible when enabling aggregation

My proposal is close to what @m4theushw is proposing

Where is the label rendered ?

  • New prop aggregationFooterLabelField to let the user decide on which column the footer label should be rendered (by default if tree data or row grouping are enabled, use their grouping column, if not then do not render any label)

What is the label content ?

  • Each aggregation function have optional shortLabel and fullLabel properties that have priority over the l10n keys below
  • Each built in aggregation function has a l10n key for a full label (Total, Minimum, Maximum, ...) that will be rendered on the column of aggregationFooterLabelField when a single aggregation function is being used
  • Each built in aggregation function has a l10n key for a short label (sum, min, max, ...) that will be rendered on the data column's header
  • New l10n key to choose the content to render on the column of aggregationFooterLabelField when several aggregation functions are being used

Possible evolution in the future

  • New prop aggregationColumnHeaderLabelEnabled (if you have a better name), if true then the header name is wrapped with the name of the aggregation function (sum(Gross) for instance). Question: how would it work when using both inline and footer aggregation on the same column ?
  • New prop to allow to render the short label directly on the footer cell and/or show it in a tooltip
  • New prop to allow to switch to Notion's mode with the aggregation function select directly in the footer cell

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Jun 1, 2022

New prop aggregationFooterLabelField to let the user decide on which column the footer label should be rendered (by default if tree data or row grouping are enabled, use their grouping column, if not then do not render any label)

This might cover the basic needs on the developer side.
But given the flexibility of this feature, I think we should focus on giving the better out-of-the-box to the end-user as well.

If we're going in this direction, I'd at least go for C (adding automatic labels if multiple aggregation functions are used).
But then again, I'd prefer B just to have consistent behavior.

I believe Aggregation should be more driven towards the end-user than the developer (not meaning to block customization or anything like that).
Why? The different use cases that can emerge with this feature is IMO what will bring the most value to the users, and might be beyond the predictions of the developers. You can literally do some operations comparable to complex SQL, locally with a few clicks. And I'm betting end-users will soon after release figure that out.

@flaviendelangle
Copy link
Member

@joserodolfofreitas

I don't follow you.
How does aggregationFooterLabelField prevents the end user from achieving something ?
If we go with a "Solution A"-type implementation, this prop is only here to define which column receives the label, not the content of the label.

If we put the label on the 1st column, we need to define what "1st column" means.
And with column pinning / row grouping / checkbox selection etc... it's very hard to decide on which columns the label should be put.
This prop is just here so allow to manually say "put the labels on the column X".


About Solution A or Solution B (and there variants).
I felt like the majority of the team was leaning towards A rather than B but I can write a message on Slack to be sure.

Those are the two major directions we can take.

I prefer A because I see how to improve it to feel the main gap in the future which is the label on multi aggregation.
I see 3 improvements possible:

  • adding a header name label like AG-Grid does
  • adding a tooltip on the aggregation cell like @m4theushw proposed
  • adding a prop addLabelToAggregationCellWhenMultiAggregation (terrible name) to switch to mode C,
  • etc...

But for B, I don't see how to fix the problems. For instance the fact that the column will have a be significantly larger when aggregation is used, I don't have any solution for it.

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Jun 1, 2022

How does aggregationFooterLabelField prevents the end user from achieving something ?

It doesn't. But it serves no purpose for the end-user directly (that's what I mean).
For me, personally, displaying somehow the "label on multi aggregation" is an important feature because it powers up the most advanced use cases, which is what the Premium features are about. That's why I'm saying it's important.
I'm fine with a step-by-step approach, but "Solution A" doesn't answer the question at all.
So I'd rather start with B and walk back, but still provide the label to multi-aggregation out of the box.

For instance the fact that the column will have a be significantly larger when aggregation is used, I don't have any solution for it.

We could try to verticalize it.

@flaviendelangle
Copy link
Member

It doesn't. But it serves no purpose for the end-user directly (that's what I mean).

I still don't understand
If we do go with the Solution A, it has the purpose of letting the developer choose where the label will be. Without it we don't know where to put the label so it does have a purpose.

I feel like the debate around this prop has nothing to do with the real discussion here.
If we do Solution A or C we need it, if we do Solution B we don't need it.


We could try to verticalize it.

If you mean, to put the label above (or below) the value, then again it's touching the layout
I don't see how to implement it properly with the features we already have (if the user says "my row is 40px high", we override this value ? If the user uses autoPageHeight, we change the page size when starting the aggregation ? And if we are not on page 0, to which page do we go to still have the same rows visible on screen ?).

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Jun 1, 2022

Considering the goal of not neglecting the labels when using multiple aggregations:

  • I'd be fine with Solution D.
  • The tooltip can play an interesting support role here, but I wouldn't use it as the main way to figure out the aggregation function.
  • The "addLabelToAggregationCellWhenMultiAggregation" could be an interesting solution but again is not in the hands of the end-user. it could be ideal if it's accessible to the end-user on the column menu somehow.

In this sense addLabelToAggregationCellWhenMultiAggregation and aggregationFooterLabelField has the same problem, they are potentially not options directly accessible to the end-user.

@flaviendelangle
Copy link
Member

For aggregationFooterLabelField I don't see the point of letting the end user choose it.
The main use case is when used without row grouping (when there is the row grouping, I guess the default value is good enough in 99% of the scenarios).

For you, when would an end user want to change the value set by the developer of the app ?


For addLabelToAggregationCellWhenMultiAggregation I agree that depending on its exact behavior, it may be interesting to let the user choose its value.
I was more describing a broad idea than an exact behavior. It could be a (controllable) state linked to an UI element in the toolbar (not the column menu, this config is global to all columns, I don't think we should support different configs depending on the column, it makes everything even more complicated).

And you could imagine other variations instead of addLabelToAggregationCellWhenMultiAggregation, like a prop aggregationLabelMode?: 'data-column' | 'grouping-column' that would be linked to an UI element in the toolbar.


I am focusing here on the developer API, because once we have this API, we can build UIs for it (and give the ability to the developers to build there own if they have special needs).


The problem of solution B is when you aggregate both inline and on footer with different functions.
Basically I love this solution when using the simplified model (this https://github.com/mui/mui-x/pull/4208/files#r886767207).
But it works poorly with the full model.

But maybe that's an interesting trade-off:

  1. In simplified mode => Solution D
  2. In full mode => Solution B

And we enforce that all model items must follow the same mode (no { gross: 'sum', year: { footer: 'min', inline: 'max' } })

Or an even more drastic decision: We don't allow to aggregate on 2 locations of the same column.
With a new prop getAggregationPosition?: (rowId: GridRowId, columnField: string, aggregationFunctionName: string) => 'footer' | 'inline'

Because

  1. I think it's very important to be able to aggregate on footer or inline depending on your dataset and configuration (if you have row grouping or not for instance).
    BUT
  2. I don't think that's something you want to change dynamically (remember that AG-Grid does not even support full aggregation on footer, maybe we can limit a bit the potential of our feature for some clarify / consistency).

@toinhao1
Copy link

Is there no way currently to inject values into the grouped rows?

For example if I have the data to be displayed in the grouped row.

@flaviendelangle
Copy link
Member

The valueGetter is called for every row, including the group rows
See this example

@ErnestBrandi
Copy link

@flaviendelangle but is it possible to get actual values of children rows (in editable mode) ? Because from what I can understand, params.rowNode.children only returns children ids.

@flaviendelangle
Copy link
Member

Once you have a row id, you can call apiRef.current.getRow to get the row model attributed to this id.

@chiendang1709
Copy link

chiendang1709 commented Jul 18, 2022

can i see an example of this, now i want to apply it to my project @flaviendelangle
177842668-bf777e42-dc84-419c-a1e7-86779cc841d6

@flaviendelangle
Copy link
Member

@chiendang1709 the feature will be available in a few weeks, we need to finish #1251 first

You have the doc page here
But for now, everything is prefixed by private_ and is not supposed to be use at all before we officially release it.

@oliviertassinari oliviertassinari added the feature: Aggregation Related to the data grid Aggregation feature label Jul 28, 2022
@githorse
Copy link

Any update on when aggregation might drop?

@flaviendelangle
Copy link
Member

The aggregation is available as an unstable feature since v5.15.0 and as a stable feature since v6.0.0

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: Aggregation Related to the data grid Aggregation feature linked in docs The issue is linked in the docs, so completely skew the upvotes new feature New feature or request plan: Premium Impact at least one Premium user
Projects
None yet
Development

Successfully merging a pull request may close this issue.