-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[DataGrid] Column management design updates #16630
Conversation
Deploy preview: https://deploy-preview-16630--material-ui-x.netlify.app/ |
width: '100%', | ||
height: '4px', | ||
animation: `${reveal} linear both`, | ||
animationTimeline: '--scroll-timeline', |
There was a problem hiding this comment.
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.
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), | ||
}, | ||
})} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
There was a problem hiding this 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
There was a problem hiding this 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor design updates to the column management panel, including: