mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.11k stars 1.27k forks source link

Aggregation Column Header Label Should be Optional #5682

Open mschaefer-gresham opened 2 years ago

mschaefer-gresham commented 2 years ago

Duplicates

Latest version

Current behavior ๐Ÿ˜ฏ

Aggregation label in column header should be optional. Providing empty label doesnโ€™t work because column titles become unaligned.

Leaving the label field off the aggregation function results in the aggregation function name being used as the label.

Using an empty string for the label doesn't work because then the column title drops slightly and doesn't line up with non-aggregated columns.

Expected behavior ๐Ÿค”

It should be possible to not display the aggregation label in the column header and for column titles to line up.

Steps to reproduce ๐Ÿ•น

Link to live example:

Steps:

  1. Create custom aggregator function.
  2. Set label to empty string.
  3. Add the aggregator to a column.
  4. See that the column title of this column doens't line to other columns.

Context ๐Ÿ”ฆ

We don't want the aggregator label to be displayed in the column header.

Your environment ๐ŸŒŽ

npx @mui/envinfo ``` System: OS: macOS 12.5 Binaries: Node: 16.3.0 - ~/.nvm/versions/node/v16.3.0/bin/node Yarn: 1.22.19 - /usr/local/bin/yarn npm: 7.20.0 - ~/.nvm/versions/node/v16.3.0/bin/npm Browsers: Chrome: 103.0.5060.134 Edge: Not Found Firefox: 86.0.1 Safari: 15.6 npmPackages: @emotion/react: ^11.10.0 => 11.10.0 @emotion/styled: ^11.10.0 => 11.10.0 @mui/base: 5.0.0-alpha.92 @mui/icons-material: ^5.8.4 => 5.8.4 @mui/lab: ^5.0.0-alpha.93 => 5.0.0-alpha.93 @mui/material: ^5.9.3 => 5.9.3 @mui/private-theming: 5.9.3 @mui/styled-engine: 5.8.7 @mui/system: ^5.9.3 => 5.9.3 @mui/types: 7.1.5 @mui/utils: 5.9.3 @mui/x-data-grid: 5.15.0 @mui/x-data-grid-generator: ^5.15.0 => 5.15.0 @mui/x-data-grid-premium: ^5.15.0 => 5.15.0 @mui/x-data-grid-pro: 5.15.0 @mui/x-license-pro: 5.15.0 @types/react: ^18.0.15 => 18.0.15 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^4.7.4 => 4.7.4 ```

Order ID ๐Ÿ’ณ (optional)

45466

cherniavskii commented 2 years ago

Great, first feedback on the aggregation feature! cc @flaviendelangle

BTW @joserodolfofreitas this is a good case for "enhancement" label, since "new feature" label doesn't feel right here.

flaviendelangle commented 2 years ago

Hi, Thanks for your contribution.

Could you provide a working reproduction, this Codesandbox template might be a good starting point.


What is your use case ? Are you trying to hide all the aggregation label in the headers ?

mschaefer-gresham commented 2 years ago

mui-testing.zip

Yes, we want to hide the aggregator label in all the headers.

joserodolfofreitas commented 2 years ago

I see an opportunity for this option when the grid is statically/programmatically aggregating the data. In this case, it's probably clear to the users that the summary footer will always be a single aggregated function, for instance, sum, or in the case of @mschaefer-gresham, a custom function.

github-actions[bot] commented 2 years ago

Since the issue is missing key information and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

mschaefer-gresham commented 2 years ago

Can you please reopen this issue. I provided clear instructions for how to reproduce this issue and even a working reproduction (even though it couldn't be more straightforward to reproduce. Just remove the label from the custom aggregator example in the documenation). I also responded to the inqueries about our use case.

We will always apply the same custom aggregator function for numeric columns and we disable the column settings menu in the header. It must be possible to disable/hide the aggregator function label in the column header since (in our case) it is just not needed and is in fact a blemish on the design of our application.

cherniavskii commented 2 years ago

Hey @mschaefer-gresham I've created a demo on Codesandbox from the one you provided just to make it easier to see it in action: https://codesandbox.io/s/aggregationinitialstate-demo-mui-x-forked-r29xjk?file=/demo.tsx

flaviendelangle commented 2 years ago

OK So the need here is to totally disable the aggregation label in the header for all columns ? Or on a column by column basis ?

mschaefer-gresham commented 2 years ago

@flaviendelangle for our purposes we will always disable (i.e. hide) the aggregation label in all columns.

flaviendelangle commented 2 years ago

@cherniavskii we could add a new prop disableAggregationHeaderLabel I don't see why someone would need to disable only on certain columns, the output would be weird. Or at least that's enough of an edge case to not complexify the main api.

cherniavskii commented 2 years ago

@flaviendelangle What do you think about supporting null value for label to not render label element at all in this case?

flaviendelangle commented 2 years ago

That would be for a column by column customization right ?

cherniavskii commented 2 years ago

No, I meant GridAggregationFunction['label'], that would be for all columns (but for a single aggregation function) But I just realized the same label is also used in column menu aggregation select :/

cherniavskii commented 2 years ago

We can hide the label with CSS though:

<DataGridPremium
  sx={{
    ".MuiDataGrid-aggregationColumnHeaderLabel": {
      display: "none"
    }
  }}
/>

Working demo: https://codesandbox.io/s/aggregationinitialstate-demo-mui-x-forked-5mlj6l?file=/demo.tsx:2213-2408

flaviendelangle commented 2 years ago

But I just realized the same label is also used in column menu aggregation select :/

Yeah, and that would also force users to override all the built-in aggregation methods to hide the label.

overweightdev commented 1 year ago

Hiding the label with CSS works for me, thanks! I think having an easy to use flag like disableAggregationHeaderLabel might be a nice convenience feature as well.

Also just my opinion but it seems like a strange UI choice to have the aggregate function label in the column header. If a label is needed it might make more sense having it next to the aggregate value itself I wonder. Food for thought

flaviendelangle commented 1 year ago

The place where the label is rendered was the topic of endless discussion, and I think that everyone at MUI who worked on it saw that it was a pain to find the perfect place that match all use-cases.

We ended up with the header, with the idea of maybe allowing to place it somewhere else if the user prefer it on the cell itself or on the grouping column.

The cell itself has two main downsides:

  1. With heavily-customized renderCell, it's complex to add a label
  2. The column width would have to react to the presence of the label, otherwise, all the default width provided by the user would probably not be sufficient when a label is present

The design of the header label could probably be improved as well by the way