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 menu visual updates #8424

Closed
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import * as React from 'react';
import PropTypes from 'prop-types';
import { GridColumnMenuItemProps, useGridSelector } from '@mui/x-data-grid-pro';
import MenuItem from '@mui/material/MenuItem';
import MUIMenuItem from '@mui/material/MenuItem';
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 { styled } from '@mui/material/styles';
import { useGridApiContext } from '../hooks/utils/useGridApiContext';
import { useGridRootProps } from '../hooks/utils/useGridRootProps';
import {
Expand All @@ -18,6 +17,39 @@ import {
import { gridAggregationModelSelector } from '../hooks/features/aggregation/gridAggregationSelectors';
import { GridAggregationModel } from '../hooks/features/aggregation/gridAggregationInterfaces';

const MenuItem = styled(MUIMenuItem)(() => ({
cursor: 'default',
'&:hover': {
Copy link
Member

@oliviertassinari oliviertassinari May 31, 2023

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:

Suggested change
'&: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.

Copy link
Member Author

@MBilalShafi MBilalShafi Jun 5, 2023

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.

backgroundColor: 'transparent',
},
'& .MuiFormControl-root': {
minWidth: 150,
display: 'flex',
flexDirection: 'row',
alignItems: 'center',
},
'& button': {
padding: 2,
marginLeft: 4,
},
}));

const iconSx = { marginRight: 1.5 };

function ClearIcon({
handleChange,
}: {
handleChange: (event: { target: { value: string } }) => void;
}) {
const rootProps = useGridRootProps();

return (
<rootProps.slots.baseIconButton sx={iconSx} onClick={handleChange}>
<rootProps.slots.columnMenuClearIcon fontSize="small" color="action" />
</rootProps.slots.baseIconButton>
);
}

function GridColumnMenuAggregationItem(props: GridColumnMenuItemProps) {
const { colDef } = props;
const apiRef = useGridApiContext();
Expand Down Expand Up @@ -53,7 +85,7 @@ function GridColumnMenuAggregationItem(props: GridColumnMenuItemProps) {
return '';
}, [rootProps.aggregationFunctions, aggregationModel, colDef]);

const handleAggregationItemChange = (event: SelectChangeEvent<string | undefined>) => {
const handleAggregationItemChange = (event: { target: { value: string } }) => {
const newAggregationItem = event.target?.value || undefined;
const currentModel = gridAggregationModelSelector(apiRef);
const { [colDef.field]: columnItem, ...otherColumnItems } = currentModel;
Expand All @@ -74,32 +106,36 @@ function GridColumnMenuAggregationItem(props: GridColumnMenuItemProps) {
<rootProps.slots.columnMenuAggregationIcon fontSize="small" />
</ListItemIcon>
<ListItemText>
<FormControl size="small" fullWidth sx={{ minWidth: 150 }}>
<rootProps.slots.baseFormControl size="small" fullWidth>
<InputLabel id={`${id}-label`}>{label}</InputLabel>
<Select
<rootProps.slots.baseSelect
Copy link
Member

Choose a reason for hiding this comment

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

👍

aria-labelledby={`${id}-label`}
labelId={`${id}-label`}
id={`${id}-input`}
value={selectedAggregationRule}
label={label}
color="primary"
onChange={handleAggregationItemChange}
onBlur={(e) => e.stopPropagation()}
onBlur={(e: FocusEvent) => e.stopPropagation()}
endAdornment={
selectedAggregationRule ? (
<ClearIcon handleChange={handleAggregationItemChange} />
) : null
}
fullWidth
>
<MenuItem value="">...</MenuItem>
{availableAggregationFunctions.map((aggFunc) => (
<MenuItem key={aggFunc} value={aggFunc}>
<rootProps.slots.baseSelectOption key={aggFunc} value={aggFunc}>
{getAggregationFunctionLabel({
apiRef,
aggregationRule: {
aggregationFunctionName: aggFunc,
aggregationFunction: rootProps.aggregationFunctions[aggFunc],
},
})}
</MenuItem>
</rootProps.slots.baseSelectOption>
))}
</Select>
</FormControl>
</rootProps.slots.baseSelect>
</rootProps.slots.baseFormControl>
</ListItemText>
</MenuItem>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react';
import { createRenderer, screen, userEvent, within, act } from '@mui/monorepo/test/utils';
import { createRenderer, screen, act, userEvent, within } from '@mui/monorepo/test/utils';
import { expect } from 'chai';
import { getColumnHeaderCell, getColumnValues } from 'test/utils/helperFn';
import { SinonSpy, spy } from 'sinon';
Expand Down Expand Up @@ -362,17 +362,19 @@ describe('<DataGridPremium /> - Aggregation', () => {
act(() => apiRef.current.showColumnMenu('id'));
clock.runToLast();

expect(screen.queryByLabelText('Aggregation')).not.to.equal(null);
expect(screen.getByRole('button', { name: 'Aggregation' })).not.to.equal(null);
});

it('should update the aggregation when changing "Aggregation" select value', () => {
render(<Test />);

// no aggregation by default
expect(getColumnValues(0)).to.deep.equal(['0', '1', '2', '3', '4', '5']);

// open column menu
act(() => apiRef.current.showColumnMenu('id'));
clock.runToLast();
userEvent.mousePress(screen.getByLabelText('Aggregation'));
userEvent.mousePress(screen.getByRole('button', { name: 'Aggregation' }));
userEvent.mousePress(
within(
screen.getByRole('listbox', {
Expand All @@ -381,6 +383,7 @@ describe('<DataGridPremium /> - Aggregation', () => {
).getByText('max'),
);

// aggregation should be applied
expect(getColumnValues(0)).to.deep.equal(['0', '1', '2', '3', '4', '5', '5' /* Agg */]);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ function GridColumnHeaderMenu({
target={target}
onClickAway={hideMenu}
onExited={onExited}
className="mui-fixed"
>
<ContentComponent
colDef={colDef}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ import { isHideMenuKey, isTabKey } from '../../../utils/keyboardUtils';
import { GridColumnMenuContainerProps } from './GridColumnMenuProps';
import { gridClasses } from '../../../constants/gridClasses';

const StyledMenuList = styled(MenuList)(() => ({
minWidth: 248,
const StyledMenuList = styled(MenuList)(({ theme }) => ({
[theme.breakpoints.up('sm')]: {
minWidth: 248,
},
}));

const GridColumnMenuContainer = React.forwardRef<HTMLUListElement, GridColumnMenuContainerProps>(
Expand Down