Skip to content

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Jul 29, 2025

@cherniavskii cherniavskii added type: bug It doesn't behave as expected. scope: data grid Changes related to the data grid. plan: Premium Impact at least one Premium user. feature: Row grouping Related to the data grid Row grouping feature labels Jul 29, 2025
@mui-bot
Copy link

mui-bot commented Jul 29, 2025

Deploy preview: https://deploy-preview-18967--material-ui-x.netlify.app/

Updated pages:

Bundle size report

Bundle Parsed size Gzip size
@mui/x-data-grid 🔺+64B(+0.01%) 🔺+10B(+0.01%)
@mui/x-data-grid-pro 🔺+64B(+0.01%) 🔺+8B(0.00%)
@mui/x-data-grid-premium 🔺+372B(+0.04%) 🔺+100B(+0.04%)
@mui/x-charts 0B(0.00%) 0B(0.00%)
@mui/x-charts-pro 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers-pro 0B(0.00%) 0B(0.00%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 4979c6e

@cherniavskii cherniavskii added the feature: Pivoting Related to the data grid pivoting feature label Jul 29, 2025
Comment on lines +131 to +132
// In single column grouping mode, the `valueFormatter` of the grouping column uses
// value formatters from original columns for each of the grouping criteria
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 think this is the only way valueFormatter could work in rowGroupingColumnMode="single".

@cherniavskii cherniavskii marked this pull request as ready for review July 29, 2025 18:32
@cherniavskii cherniavskii requested a review from arminmeh July 29, 2025 18:32
@arminmeh
Copy link
Contributor

This still fails for this reproduction code

import * as React from 'react';
import { DataGridPremium, GridColDef, GridRowGroupingModel } from '@mui/x-data-grid-premium';

const columns: GridColDef[] = [
  { field: 'id', headerName: 'ID' },
  {
    field: 'transactionDate',
    type: 'date',
    headerName: 'Transaction Date',
    valueGetter: (value) => new Date(value),
    groupingValueGetter: (value) => value,
  },
  { field: 'ticker', headerName: 'Ticker' },
];

const rowGroupingModel: GridRowGroupingModel = ['transactionDate'];

export default function GridGetPivotDerivedColumns() {
  return (
    <div style={{ width: '100%' }}>
      <div style={{ height: 300, width: '100%' }}>
        <DataGridPremium
          rows={rows}
          columns={columns}
          initialState={{
            rowGrouping: {
              model: rowGroupingModel,
            },
          }}
        />
      </div>
    </div>
  );
}

const rows = [
  {
    id: 1,
    transactionDate: '2024-05-15',
    ticker: 'AAPL',
    price: 192.45,
    volume: 5500,
    type: 'stock',
  },
];

IIUC the problem is that we are using groupingValueGetter() to generate the group key, but also it's value.
Since the requirement is that the key is string | number | null | undefined it can't be a Date which is a requirement for the date column.

We could return here
https://github.com/mui/mui-x/blob/v8.9.2/packages/x-data-grid-premium/src/hooks/features/rowGrouping/gridRowGroupingUtils.ts#L237
together with the key the formatted value as
value: apiRef.current.getRowFormattedValue(row, colDef),
and later use the key for the grouping path and the value for the actual value

WDYT?

@cherniavskii
Copy link
Member Author

@arminmeh I'm not sure I understand your proposal, can you provide more details? Or feel free to push to this branch so we can give it a try

@arminmeh
Copy link
Contributor

arminmeh commented Aug 1, 2025

@arminmeh I'm not sure I understand your proposal, can you provide more details? Or feel free to push to this branch so we can give it a try

I've made an example here
#19005

it is still missing few pieces, but it covers the discussion here

I have updated the transactionDate field from my previous example to

{
  field: 'transactionDate',
  type: 'date',
  headerName: 'Transaction Date',
  width: 200,
  valueGetter: (value) => new Date(value),
  valueFormatter: (value: Date) => `F${value}`,
  groupingValueGetter: (value) => `G${value}`,
},

for easier testing.

Running this on master actually works now (valueFormatter is added), but it combines return values from both valueFormatter and groupingValueGetter to create the final content of the grouped field.

With the changes from the linked PR, the value from groupingValueGetter is only used to create a grouping key, but it is not displayed to the user - valueGetter and valueFormatter can be used for that.

The problem is that, even though we say that groupingValueGetter should be used to convert complex values to a valid grouping value, people are probably using it for the value formatting, which means that my update would probably break some apps.

We could either postpone this update for v9 or have another prop like getGroupingKey and deprecate groupingValueGetter

IMO the updated behavior is better, because you use the same props for formatting leafs and group rows

@cherniavskii
Copy link
Member Author

@arminmeh It still throws an error if you change the grouping model to this:

const rowGroupingModel: GridRowGroupingModel = ['ticker', 'transactionDate'];

I think we need this change to solve it.

We could either postpone this update for v9 or have another prop like getGroupingKey and deprecate groupingValueGetter

How about adding optional groupingValueFormatter?

const rowId = gridRowIdSelector(apiRef, row);
const rowNode = gridRowNodeSelector(apiRef, rowId);
if (rowNode.type === 'group') {
const originalColDef = originalColDefs.get(rowNode.groupingField!)!;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be
columnsLookup[rowNode.groupingField]
so that you avoid building originalColDefs?

@arminmeh
Copy link
Contributor

arminmeh commented Aug 6, 2025

@arminmeh It still throws an error if you change the grouping model to this:

const rowGroupingModel: GridRowGroupingModel = ['ticker', 'transactionDate'];

I think we need this change to solve it.

We could either postpone this update for v9 or have another prop like getGroupingKey and deprecate groupingValueGetter

How about adding optional groupingValueFormatter?

Let's merge this one then to fix the user problem.
I will link this comment to the v9 discussion umbrella issue and we can see if the API needs to be updated in the next major

Copy link
Contributor

@arminmeh arminmeh left a comment

Choose a reason for hiding this comment

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

Still failing for
#18967 (comment)

Copy link
Contributor

@arminmeh arminmeh left a comment

Choose a reason for hiding this comment

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

Still failing for
#18967 (comment)

field: 'composer',
headerName: 'Composer',
valueGetter: (value) => value.name,
groupingValueGetter: (value) => value.name,
Copy link
Member Author

Choose a reason for hiding this comment

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

@arminmeh
I think I fixed all use cases, but now the valueGetter is being called first, and the result is passed to groupingValueGetter (if provided). What do you think about this approach?

This might break for someone, but it's kindof a bugfix.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be confusing, since value can either be a value from the row or the return value from the column's valueGetter.

As mentioned before, the problem is that we are using groupingValueGetter() to generate the group key and the column value.
Since the requirement is that the key is string | number | null | undefined, it can't be a Date, which is a requirement for the date column.

So, we could update the default formatter
https://github.com/mui/mui-x/blob/v8.10.2/packages/x-data-grid/src/colDef/gridDateColDef.ts#L30
not to check for Date if the row is autogenerated.

Copy link
Member Author

Choose a reason for hiding this comment

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

not to check for Date if the row is autogenerated

Hmm, I'll try that

headerName: 'Transaction Date',
width: 140,
valueGetter: (value) => new Date(value),
groupingValueGetter: (value) => value,
Copy link
Contributor

@arminmeh arminmeh Aug 28, 2025

Choose a reason for hiding this comment

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

Should we update the docs at https://mui.com/x/react-data-grid/row-grouping/#using-groupingvaluegetter-for-complex-grouping-value to state that Date is also allowed as a grouping value?

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.
Also updated the demo for groupingValueGetter, because now groupingValueGetter is not necessary if you have valueGetter, unless you want to use a different value for grouping than the one in the cell.
https://deploy-preview-18967--material-ui-x.netlify.app/x/react-data-grid/row-grouping/#using-groupingvaluegetter-for-complex-grouping-value
WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is a nice addition

import { renderEditDateCell } from '../components/cell/GridEditDateCell';
import { gridRowIdSelector } from '../hooks/core/gridPropsSelectors';
import { isAutogeneratedRow } from '../hooks/features/rows/gridRowsUtils';
import { isGroupingColumn } from '../internals/utils/gridRowGroupingUtils';
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to remove this check because the formatter gets the original column definition.
Checking for autogenerated row should be enough.

@cherniavskii cherniavskii merged commit 22f5b49 into mui:master Aug 28, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: Pivoting Related to the data grid pivoting feature feature: Row grouping Related to the data grid Row grouping feature plan: Premium Impact at least one Premium user. scope: data grid Changes related to the data grid. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[data grid] Error when "Transaction Date" (type: date) is added to Rows section in Pivot Panel

3 participants