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.

Sign up for GitHub

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
wants to merge 11 commits into from
+60 −18
Closed

[DataGrid] Column menu visual updates #8424

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ef48445
[DataGrid] Fix aggregation item causes the body scroll to jump
MBilalShafi Mar 29, 2023
d9696a0
Explore a solution with native select
MBilalShafi Mar 30, 2023
8993b38
Remove width constraint on mobile devices
MBilalShafi Mar 31, 2023
39dd471
Use baseSelect and fix tests
MBilalShafi Mar 31, 2023
ef94a5e
Remove dangling .only
MBilalShafi Mar 31, 2023
2920fb2
Re-run CI
MBilalShafi Apr 4, 2023
3f7d3c1
Merge branch 'master' into fix-aggregation-item-column-menu
MBilalShafi May 17, 2023
ef12f16
Improve column menu aggregation item
MBilalShafi May 17, 2023
71038a3
Merge branch 'master' into fix-aggregation-item-column-menu
MBilalShafi Jun 5, 2023
bda19e7
Olivier's feedback
MBilalShafi Jun 5, 2023
1c7a05d
Fix test
MBilalShafi Jun 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 32 additions & 12 deletions packages/grid/x-data-grid-premium/src/components/GridColumnMenuAggregationItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
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 type { 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 +18,22 @@ import {
import { gridAggregationModelSelector } from '../hooks/features/aggregation/gridAggregationSelectors';
import { GridAggregationModel } from '../hooks/features/aggregation/gridAggregationInterfaces';

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

@oliviertassinari oliviertassinari May 31, 2023 •
edited
Loading

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 •
edited
Loading

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,
},
}));

function GridColumnMenuAggregationItem(props: GridColumnMenuItemProps) {
const { colDef } = props;
const apiRef = useGridApiContext();
Expand Down Expand Up @@ -74,32 +90,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()}
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>
{selectedAggregationRule ? (
<rootProps.slots.baseIconButton onClick={handleAggregationItemChange}>
<rootProps.slots.columnMenuClearIcon fontSize="small" color="action" />
</rootProps.slots.baseIconButton>
) : null}
Copy link
Member

@oliviertassinari oliviertassinari May 31, 2023 •
edited
Loading

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

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).

Copy link
Member Author

@MBilalShafi MBilalShafi Jun 5, 2023 •
edited
Loading

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? 🤔
image

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?

</rootProps.slots.baseFormControl>
</ListItemText>
</MenuItem>
);
Expand Down
18 changes: 8 additions & 10 deletions packages/grid/x-data-grid-premium/src/tests/aggregation.DataGridPremium.test.tsx
View file
Open in desktop
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, fireEvent } 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,25 +362,23 @@ describe('<DataGridPremium /> - Aggregation', () => {
act(() => apiRef.current.showColumnMenu('id'));
clock.runToLast();

expect(screen.queryByLabelText('Aggregation')).not.to.equal(null);
expect(screen.getByLabelText('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(
within(
screen.getByRole('listbox', {
name: 'Aggregation',
}),
).getByText('max'),
);

// select aggregation function 'max'
fireEvent.change(screen.getByRole('combobox'), { target: { value: 'max' } });

// aggregation should be applied
expect(getColumnValues(0)).to.deep.equal(['0', '1', '2', '3', '4', '5', '5' /* Agg */]);
});
});
Expand Down
1 change: 1 addition & 0 deletions packages/grid/x-data-grid/src/components/menu/columnMenu/GridColumnHeaderMenu.tsx
View file
Open in desktop
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
4 changes: 3 additions & 1 deletion packages/grid/x-data-grid/src/components/menu/columnMenu/GridColumnMenuContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import { GridColumnMenuContainerProps } from './GridColumnMenuProps';
import { gridClasses } from '../../../constants/gridClasses';

const StyledMenuList = styled(MenuList)(() => ({
minWidth: 248,
'@media (pointer: fine)': {
Copy link
Member

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

Suggested change
'@media (pointer: fine)': {
[theme.breakpoints.up('sm')]: {

Copy link
Member Author

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.

minWidth: 248,
},
}));

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