-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 menu visual updates #8424
base: v6.x
Are you sure you want to change the base?
[DataGrid] Column menu visual updates #8424
Conversation
Netlify deploy previewNetlify deploy preview: https://deploy-preview-8424--material-ui-x.netlify.app/ Updated pagesNo updates. These are the results for the performance tests:
|
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!
value={selectedAggregationRule} | ||
label={label} | ||
native |
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 think we can avoid using native
here and fix the column menu shift with mui-fixed
class - see https://mui.com/material-ui/getting-started/faq/#why-do-the-fixed-positioned-elements-move-when-a-modal-is-opened
diff --git a/packages/grid/x-data-grid/src/components/menu/columnMenu/GridColumnHeaderMenu.tsx b/packages/grid/x-data-grid/src/components/menu/columnMenu/GridColumnHeaderMenu.tsx
index 5c067a68b..a7027d560 100644
--- a/packages/grid/x-data-grid/src/components/menu/columnMenu/GridColumnHeaderMenu.tsx
+++ b/packages/grid/x-data-grid/src/components/menu/columnMenu/GridColumnHeaderMenu.tsx
@@ -51,6 +51,7 @@ function GridColumnHeaderMenu({
target={target}
onClickAway={hideMenu}
onExited={onExited}
+ className="mui-fixed"
>
<ContentComponent
colDef={colDef}
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.
Opened an issue in the core repo: mui/material-ui#37207
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.
I can't reproduce this one on macOS & Chrome.
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.
Correct, mostly it's reproducible on Firefox (Mac OS), to reproduce on Chrome, I had to follow these steps:
- Open aggregation menu item
- Right-click to open the context menu
- Click on some other window, say VS Code, to defocus the browser.
Maybe the case is very less likely, so ignore it?
@@ -76,29 +83,30 @@ function GridColumnMenuAggregationItem(props: GridColumnMenuItemProps) { | |||
<ListItemText> | |||
<FormControl size="small" fullWidth sx={{ minWidth: 150 }}> | |||
<InputLabel id={`${id}-label`}>{label}</InputLabel> | |||
<Select | |||
<rootProps.slots.baseSelect |
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.
👍
import ListItemIcon from '@mui/material/ListItemIcon'; | ||
import ListItemText from '@mui/material/ListItemText'; | ||
import FormControl from '@mui/material/FormControl'; | ||
import InputLabel from '@mui/material/InputLabel'; | ||
import { unstable_useId as useId } from '@mui/utils'; | ||
import Select, { SelectChangeEvent } from '@mui/material/Select'; | ||
import { SelectChangeEvent } from '@mui/material/Select'; |
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.
Maybe replace SelectChangeEvent
with { target: { value: string } }
to avoid imports from @mui/material/Select
?
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.
Why I kept it here is, I thought most transpilers strip off type imports on production builds, due to which it should be OK to use types from these packages, am I missing something here?
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 think that the point Andrew raised is that this import will fail once we make @mui/material an optional peer dependency. I think it can be solved later on.
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 I see, let's remove it then
@@ -18,6 +18,22 @@ import { | |||
import { gridAggregationModelSelector } from '../hooks/features/aggregation/gridAggregationSelectors'; | |||
import { GridAggregationModel } from '../hooks/features/aggregation/gridAggregationInterfaces'; | |||
|
|||
const MenuItem = styled(MUIMenuItem)(() => ({ | |||
'&:hover': { |
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.
The pointer cursor feels strange, it signals that I can click but clicking doesn't do anything:
Screen.Recording.2023-05-31.at.15.29.43.mov
So either:
'&:hover': { | |
cursor: 'default', | |
'&:hover': { |
or we wire the click to focus the select and open it.
Off-topic. The MenuItem component might need a mode where it's only here to handle consistent spacing but not have any actions.
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.
Added a default cursor.
The MenuItem component might need a mode where it's only here to handle consistent spacing but not have any actions.
Yes, I guess that could be a useful mode to handle more use-cases like this.
import ListItemIcon from '@mui/material/ListItemIcon'; | ||
import ListItemText from '@mui/material/ListItemText'; | ||
import FormControl from '@mui/material/FormControl'; | ||
import InputLabel from '@mui/material/InputLabel'; | ||
import { unstable_useId as useId } from '@mui/utils'; | ||
import Select, { SelectChangeEvent } from '@mui/material/Select'; | ||
import { SelectChangeEvent } from '@mui/material/Select'; |
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 think that the point Andrew raised is that this import will fail once we make @mui/material an optional peer dependency. I think it can be solved later on.
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.
Thanks for the follow-up on my feedback in #6619!
@@ -9,7 +9,9 @@ import { GridColumnMenuContainerProps } from './GridColumnMenuProps'; | |||
import { gridClasses } from '../../../constants/gridClasses'; | |||
|
|||
const StyledMenuList = styled(MenuList)(() => ({ | |||
minWidth: 248, | |||
'@media (pointer: fine)': { |
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'm not sure that the constraint is on whether the primary pointer is fine (e.g. cursor) or not (e.g. touch screen). It feels like it's more a problem with screen width, so
'@media (pointer: fine)': { | |
[theme.breakpoints.up('sm')]: { |
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.
Makes sense, I think the same is done in MUI MenuList.
{selectedAggregationRule ? ( | ||
<rootProps.slots.baseIconButton onClick={handleAggregationItemChange}> | ||
<rootProps.slots.columnMenuClearIcon fontSize="small" color="action" /> | ||
</rootProps.slots.baseIconButton> | ||
) : null} |
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.
Should the clear icon be part of the text field? It sounds more like an external element, it's rendered outside of the input.
![Screenshot 2023-05-31 at 15 49 25](https://private-user-images.githubusercontent.com/3165635/242297294-4ff5142e-2cd6-4890-995c-0749ecc56f75.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU0MDc3MzMsIm5iZiI6MTcyNTQwNzQzMywicGF0aCI6Ii8zMTY1NjM1LzI0MjI5NzI5NC00ZmY1MTQyZS0yY2Q2LTQ4OTAtOTk1Yy0wNzQ5ZWNjNTZmNzUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwMyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDNUMjM1MDMzWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NzE4MWNmMjMxODk4ZmRmY2QyNmYwZTA5MGQ1NDUyYTY0OTI2Y2M4MGFjNDU0MzI2OGEyOGUxOWRiYzdjMmY2YyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.ccGKBTK2VzvEkVT4Vi1vaTMhFqd_rTot5VEF-kdYZzQ)
Off-topic. I wonder if we wouldn't want to move toward removing baseFormControl
, baseInputLabel
, baseSelect
as a slot. This concept feels MUI-centered, not something that translates often to custom design systems that developers might want to customize the data grid with. Instead, we could replace them with an input and select slots (not sure, but could).
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.
Yes, making it part of the text field would make it feel a bit better. endAdornment
worked but by default it collided with the dropdown icon, so I had to add a margin, is it a known bug with MUI select? 🤔
I wonder if we wouldn't want to move toward removing baseFormControl, baseInputLabel, baseSelect as a slot. This concept feels MUI-centered, not something that translates often to custom design systems that developers might want to customize the data grid with. Instead, we could replace them with an input and select slots (not sure, but could).
Do you mean native input/select components? I feel currently we are going in the other direction, by adding slots for all possible components for them to be able to be swapped between Material/Joy UI/any future design system's components. I can't picture how a developer will add their custom design system on top if we replace these items with the native input/select.
Or the point is to make it custom-interface-free (support the native props only) so that it's compatible with most users' use cases?
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Context: #6619 (comment)
Preview: https://deploy-preview-8424--material-ui-x.netlify.app/x/react-data-grid/column-menu/#column-menu-with-pro-premium-features
Changelog
minWidth
constraint from smaller screensmui-fixed
class on the menu to remove flicker on open/close