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] Add separate density factor for column headers #14022

Closed
wants to merge 2 commits into from

Conversation

KenanYusuf
Copy link
Member

@KenanYusuf KenanYusuf commented Jul 29, 2024

Adds a separate density factor for column headers. This allows us to control the size of columns headers independently from rows.

The compact density for column headers with this change is 0.8 rather than the 0.7 it was before. This means that column headers appear slightly taller now with the compact density set, buying us more space for other UI elements in the column headers like the aggregation label.

Fixes #5274

Before

Screenshot 2024-07-29 at 16 21 49

After

Screenshot 2024-07-29 at 16 21 33

@KenanYusuf KenanYusuf changed the title [DataGrid] Add separate density factor for headers [DataGrid] Add separate density factor for column headers Jul 29, 2024
@KenanYusuf KenanYusuf marked this pull request as draft July 29, 2024 10:23
@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 Jul 29, 2024
@mui-bot
Copy link

mui-bot commented Jul 29, 2024

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

Generated by 🚫 dangerJS against c1d2cc8

@@ -4,16 +4,29 @@ import { GridDensity } from '../../../models/gridDensity';

export const COMPACT_DENSITY_FACTOR = 0.7;
export const COMFORTABLE_DENSITY_FACTOR = 1.3;
export const COMPACT_DENSITY_FACTOR_HEADER = 0.8;
export const COMFORTABLE_DENSITY_FACTOR_HEADER = 1.3;
Copy link
Member Author

@KenanYusuf KenanYusuf Jul 29, 2024

Choose a reason for hiding this comment

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

We may want to lower this at some point, as comfortable column headers are quite tall, especially in column groups.

@KenanYusuf KenanYusuf marked this pull request as ready for review July 29, 2024 10:41
const headerFilterHeight = Math.floor(
(props.headerFilterHeight ?? props.columnHeaderHeight) * densityFactor,
(props.headerFilterHeight ?? props.columnHeaderHeight) * headerDensityFactor,
Copy link
Member Author

Choose a reason for hiding this comment

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

We have a separate issue to deal with the fact that compact header filters are overflowing; this change neither fixes or worsens this issue #13048

@KenanYusuf KenanYusuf requested a review from a team July 29, 2024 14:53
Copy link
Contributor

@romgrk romgrk left a comment

Choose a reason for hiding this comment

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

I don't think I like having a separate density for headers. Is there any other way to solve the issue? Could we adjust the font-size for the label instead?

@KenanYusuf
Copy link
Member Author

I don't think I like having a separate density for headers. Is there any other way to solve the issue? Could we adjust the font-size for the label instead?

@romgrk I don't think reducing the font-size is the right solution here as it would lower the importance of the labels.

We need to ensure that the header rows have enough space to accommodate all of the elements that can be rendered in it - another option is just setting a minimum height for the header row...

@romgrk
Copy link
Contributor

romgrk commented Jul 29, 2024

How about vertically centered header's content instead of aligning on the title's baseline?

image

another option is just setting a minimum height for the header row...

I also don't like it, it would still be a separate header height.

@mui/xgrid Any other opinions on this issue? I don't like separate height but I'll go with the consensus if the rest of the team is fine with it.

@KenanYusuf
Copy link
Member Author

How about vertically centered header's content instead of aligning on the title's baseline?

We have discussed this option in the Figma explorations - I don't love the misalignment but it would look better if we had persistent separators between the column headers like your example shows.

@romgrk
Copy link
Contributor

romgrk commented Jul 29, 2024

I'd be happy with persistent separators, I find our current ones a bit jarring.

@KenanYusuf KenanYusuf closed this Aug 2, 2024
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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[data grid] Improve header behavior with density property
3 participants