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] Column header design updates #14293

Merged
merged 18 commits into from
Sep 3, 2024

Conversation

KenanYusuf
Copy link
Contributor

@KenanYusuf KenanYusuf commented Aug 22, 2024

This PR includes a few small design updates to the column headers.

Preview: https://deploy-preview-14293--material-ui-x.netlify.app/x/react-data-grid/#mit-license-free-forever

1. Aggregation label type and layout updates

Before After
localhost_3001_playground_ (3) localhost_3001_playground_ (2)

2. Column separator updates

Before After
localhost_3001_playground_ (3) localhost_3001_playground_
localhost_3001_playground_ (5) localhost_3001_playground_ (1)
localhost_3001_playground_ (2) localhost_3001_playground_ (4)

3. Fixed overlapping of column menu button and resize handle

  • Reduced the resize handle hit area from 24px to 10px and repositioned the column menu buttons to avoid overlapping of the two actions.
  • Also made column menu icon size consistent with the sort icon by switching the size prop from small to inherit.
Before After
Screenshot 2024-08-22 at 15 35 38 Screenshot 2024-08-22 at 15 41 54

@KenanYusuf KenanYusuf added component: data grid This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer labels Aug 22, 2024
@mui-bot
Copy link

mui-bot commented Aug 22, 2024

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

Generated by 🚫 dangerJS against 8051e7c

@KenanYusuf KenanYusuf marked this pull request as ready for review August 23, 2024 15:56
@KenanYusuf KenanYusuf requested a review from a team August 23, 2024 16:00
'&:hover': {
color: (t.vars || t).palette.text.primary,
// Reset on touch devices, it doesn't add specificity
// Always appear as draggable on touch devices
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

Copy link
Member

Choose a reason for hiding this comment

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

Side note: Sometimes Argos diff surprises me – it doesn't pick up the separator 😅

Screen.Recording.2024-08-23.at.23.36.55.mov

Copy link
Member

Choose a reason for hiding this comment

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

Also, this PR warrants a minor release.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 27, 2024
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

The general direction looks right 👍

Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

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

Nice work!
It also reminded me of a similar issue on the header filters with compact density.
Opened a separate PR for it.

fontWeight: theme.typography.fontWeightMedium,
color: (theme.vars || theme).palette.primary.dark,
textTransform: 'uppercase',
lineHeight: 'normal',
Copy link
Member

@MBilalShafi MBilalShafi Aug 28, 2024

Choose a reason for hiding this comment

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

I'm wondering if there is a possibility some users customizing it using theme.*.* variables. What do you think? 🤔

Copy link
Contributor Author

@KenanYusuf KenanYusuf Aug 28, 2024

Choose a reason for hiding this comment

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

I'm purposefully deviating from the theme.typography.caption.lineHeight value here because it is too large. normal line-height should keep the spacing looking ok between the column header title and aggregation label I think.

If users want to override it, they have the class I guess?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, but I'm only worried if some existing users' styles would break with this.
It's obviously not very likely but theoretically, some users may be using theme.typography.caption.lineHeight to override this style.

It's a nitpick though. Feel free to merge it in current shape.

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.

Looks good to me!
I prefer not to include this PR in this week's release with the Material UI v6 support. Keeping other changes to a minimum would make the upgrade easier for those who need Material UI v6.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 1, 2024

#5274, #9162, #9776 might be related to this PR.

@KenanYusuf KenanYusuf merged commit bb26d25 into mui:master Sep 3, 2024
18 checks passed
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! design This is about UI or UX design, please involve a designer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
Status: Done
6 participants