[DataGrid] Column menu visual updates #8424
Conversation
MBilalShafi
added
component: data grid
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:
|
MBilalShafi
changed the title
| value={selectedAggregationRule} | ||
| label={label} | ||
| native |
There was a problem hiding this comment.
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.
Opened an issue in the core repo: mui/material-ui#37207
There was a problem hiding this comment.
I couldn't guess the reason but it got reproduced again on opening the context menu
There was a problem hiding this comment.
I can't reproduce this one on macOS & Chrome.
There was a problem hiding this comment.
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 | |||
| 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.
Maybe replace SelectChangeEvent with { target: { value: string } } to avoid imports from @mui/material/Select?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
| @@ -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.
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.
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.
Should the clear icon be part of the text field? It sounds more like an external element, it's rendered outside of the input.
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.
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?
oliviertassinari
added
design
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
It has been diverged a bit. Will reopen from recent commits. |
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
minWidthconstraint from smaller screensmui-fixedclass on the menu to remove flicker on open/close