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 management design updates #16630

Merged
merged 5 commits into from
Feb 19, 2025

Conversation

KenanYusuf
Copy link
Member

@KenanYusuf KenanYusuf commented Feb 17, 2025

Minor design updates to the column management panel, including:

  • Added subtle scroll shadows to indicate when the list of columns is scrollable
  • Reduced font-size of checkbox labels and empty text
  • Made header, body and footer padding consistent
  • Reduced search icon size to match other icon sizes in grid
  • Made it so that the footer is visible when the empty state is displayed, no need to hide it
Before After
before after
empty-before empty-after

@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 feature: Column visibility v8.x labels Feb 17, 2025
@mui-bot
Copy link

mui-bot commented Feb 17, 2025

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

Generated by 🚫 dangerJS against 28ac9c6

width: '100%',
height: '4px',
animation: `${reveal} linear both`,
animationTimeline: '--scroll-timeline',
Copy link
Member Author

@KenanYusuf KenanYusuf Feb 17, 2025

Choose a reason for hiding this comment

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

Uses animation-timeline to show scroll shadows.

The support for animation-timeline isn't the best, but these scroll shadows can be a progressive enhancement as they are only a minor UX improvement. Not worth implementing with JS.

@KenanYusuf KenanYusuf requested review from noraleonte and a team February 17, 2025 16:52
Comment on lines -245 to +252
sx={fullWidth ? { width: '100%', margin: 0 } : undefined}
sx={(theme) => ({
gap: 0.5,
margin: 0,
width: fullWidth ? '100%' : undefined,
[`& .${formControlLabelClasses.label}`]: {
fontSize: theme.typography.pxToRem(14),
},
})}
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried avoiding having an sx prop on the checkbox rendered in the grid cells because that property is very slow (at least until I have time to finish this, after that it will merely be slow), and it degrades the scrolling UX.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to know—I guess this change won't impact scrolling performance since it only adds the sx prop for checkboxes with labels?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, yes it should not affect scrolling performance in that case.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 18, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

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 update 👍

@@ -254,6 +255,7 @@ function GridColumnsManagement(props: GridColumnsManagementProps) {
}
tabIndex={-1}
onClick={handleSearchReset}
edge="end"
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this spacing consistent too?

SCR-20250218-oels

Copy link
Member Author

@KenanYusuf KenanYusuf Feb 18, 2025

Choose a reason for hiding this comment

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

Good spot. It looks like having an IconButton with size="small as an input adornment is problematic as it doesn't get positioned correctly. Using a regular sized icon button results in this:

Screenshot 2025-02-18 at 14 10 20

The spacing on the end adornment is closer to the start adornment's spacing, but still not perfect:

Screenshot 2025-02-18 at 14 11 21

Copy link
Contributor

Choose a reason for hiding this comment

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

The only issue I see with setting the IconButton size to regular is that on hover it looks like this:
image
Could we work around the positioning of a small IconButton instead? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we work around the positioning of a small IconButton instead?

Yeah, this is a better option, we can target small icon buttons specifically within end adornments to fix this 28ac9c6. Also fixes it in the quick filter 🎉

@@ -303,6 +306,7 @@ function GridColumnsManagement(props: GridColumnsManagementProps) {
onClick={() => toggleAllColumns(!allHideableColumnsVisible)}
name={apiRef.current.getLocaleText('columnsManagementShowHideAllText')}
label={apiRef.current.getLocaleText('columnsManagementShowHideAllText')}
density="compact"
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to set it (and other styling like spacing etc) according to rootProps.density?

Copy link
Member Author

Choose a reason for hiding this comment

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

Potentially, yes. I am thinking about how the other UI elements should respond to the density setting but it's going to require a bit of experimentation. Will create a follow up issue to review this across the board.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 18, 2025
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 great!

Copy link
Contributor

@noraleonte noraleonte left a comment

Choose a reason for hiding this comment

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

Awesome work 🚀 Looks great 💙

@@ -254,6 +255,7 @@ function GridColumnsManagement(props: GridColumnsManagementProps) {
}
tabIndex={-1}
onClick={handleSearchReset}
edge="end"
Copy link
Contributor

Choose a reason for hiding this comment

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

The only issue I see with setting the IconButton size to regular is that on hover it looks like this:
image
Could we work around the positioning of a small IconButton instead? 🤔

@KenanYusuf KenanYusuf merged commit dbc7561 into mui:master Feb 19, 2025
18 checks passed
@KenanYusuf KenanYusuf deleted the column-management-design-updates branch February 19, 2025 11:03
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 feature: Column visibility v8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants