[DataGrid] Update aggregation label styling #13950
Conversation
KenanYusuf
added
component: data grid
|
Deploy preview: https://deploy-preview-13950--material-ui-x.netlify.app/ |
|
Isn't the blue color used to connect the aggregation function name to the aggregation value at the bottom? I like the casing change but I'd keep the color as it is. |
@romgrk I think that was the intention, but I don't think the connection is at all obvious. I think the primary color should be reserved for highlighting important information (as it does for the aggregation value), and interactive elements like links and buttons. |
There was a problem hiding this comment.
Overall, I'm fine with the color change. It's worth mentioning that the usage of the accent color for aggregation wasn't super consistent – e.g. it's only used for the footer aggregation values, but not for the inline values – see examples in https://deploy-preview-13950--material-ui-x.netlify.app/x/react-data-grid/aggregation/#usage-with-row-grouping
The color change warrants a minor release IMO.
@joserodolfofreitas What do you think?
There was a problem hiding this comment.
I believe we can trust the designers' take on this. Both Kenan and Nora agree that the accent color doesn't provide a clear connection and that the color generates even more confusion in terms of UX.
We never got a ticket regarding this, though, so maybe it doesn't matter that much.
Let's see how users will receive this update.
| fontWeight: theme.typography.fontWeightMedium, | ||
| color: (theme.vars || theme).palette.primary.dark, | ||
| textTransform: 'uppercase', | ||
| top: '50%', |
There was a problem hiding this comment.
Perhaps we can move the label down a bit to avoid a potential collision with the column header text?
WDYT @KenanYusuf ?
In case of a custom aggregation label
There was a problem hiding this comment.
I agree, it could be moved down more.
The problem with that is the available space on density="compact"
As it stands, the aggregation label is already touching the bottom border when the label has a descender e.g. avg.
I would propose that we create a separate set of density factor values for column headers to ensure that our header cells always have enough space for the various elements including the aggregation label. What do you think @cherniavskii
We have an issue open already to look into this problem #5274, but I could do it as part of this PR.
There was a problem hiding this comment.
Made a separate PR for the above #14022 - I am going to hold off merging this PR until that one is merged so that we can increase the spacing a little more.
|
Going to re-open a PR with these changes and some other updates to column headers soon. |
Fixes #5530 and #9104
Problem
Aggregation labels are currently quite prominent in the header cell due to their color, font-weight and transformation to uppercase - this doesn't make sense hierarchically as they should not have equal or more importance than the header title itself.
The other issue we have is that the aggregation label is too close to the header title when the grid
densityis set tocompact.Solution
This PR updates the aggregation label style to use the secondary text color, regular font-weight, and removes the text transformation.
Removing the text transformation also makes the casing for the aggregation consistent with how the option is displayed in the column menu select.
Note: ideally there would be more space below the aggregation label, too, as it is too close to the border. Perhaps we could solve that here, or separately as part of this issue? #5274
Preview: https://deploy-preview-13950--material-ui-x.netlify.app/x/react-data-grid/aggregation/