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] Improved column menu design and api #6619

Merged
merged 114 commits into from
Dec 23, 2022

Conversation

MBilalShafi
Copy link
Member

@MBilalShafi MBilalShafi commented Oct 24, 2022

Fixes #7025
Fixes #4929

Preview: https://deploy-preview-6619--material-ui-x.netlify.app/x/react-data-grid/column-menu/#column-menu-with-pro-premium-options

Changelog

  • New column menu design with improved look and feel
  • Ability to add new menu items and place them on any order
  • Replaced all the items to be injected via pipe processor for better control through respective hooks
  • Added ability to override/reorder column menu items which may help add more items in any order e.g. for [data grid] Add an option to clear column contents #5307
  • Ability to customize the menu by passing a component in column menu slot
  • New l10n labels added
  • Added dedicated component and icon slots for Pro and Premium packages for shipping only required components per package
  • Fix accessibility bug of focus shifting to body when an item gets hidden

Breaking changes

  • column and currentColumn is now renamed to colDef

  • DATA_GRID_DEFAULT_SLOTS_COMPONENTS export has been removed

  • Some of the components have been renamed for consistency.

    Community Package:

    - <GridFilterMenuItem />
    + <GridColumnMenuFilterItem />
    - <HideGridColMenuItem />
    + <GridColumnMenuHideItem />
    - <GridColumnsMenuItem />
    + <GridColumnMenuColumnsItem />
    - <SortGridMenuItems />
    + <GridColumnMenuSortItem />
    - interface GridFilterItemProps
    + interface GridColumnMenuItemProps

    Pro package:

    - <GridColumnPinningMenuItems />
    + <GridColumnMenuPinningItem />

    Premium package:

    - <GridAggregationColumnMenuItem />
    + <GridColumnMenuAggregationItem />
    - <GridRowGroupingColumnMenuItems />
    - <GridRowGroupableColumnMenuItems />
    + <GridColumnMenuGroupingItem />

Design

@MBilalShafi MBilalShafi added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request feature: Other labels Oct 24, 2022
@mui-bot
Copy link

mui-bot commented Oct 24, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-6619--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 683.5 1,076.8 707.4 860.56 150.949
Sort 100k rows ms 628.9 1,310.2 913.8 956.5 216.608
Select 100k rows ms 235.6 363.2 248.9 281.74 49.229
Deselect 100k rows ms 156.7 294.3 230 233.8 47.351

Generated by 🚫 dangerJS against 07c4472

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 2, 2022
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 3, 2022
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Looks great, we are almost there! 👍
Would be great to get Gerda's feedback on it! cc @gerdadesign

docs/data/data-grid/column-menu/column-menu.md Outdated Show resolved Hide resolved
docs/data/data-grid/column-menu/column-menu.md Outdated Show resolved Hide resolved
docs/data/data-grid/column-menu/column-menu.md Outdated Show resolved Hide resolved
docs/data/data-grid/column-menu/column-menu.md Outdated Show resolved Hide resolved
docs/data/data-grid/column-menu/CustomColumnMenuGrid.tsx Outdated Show resolved Hide resolved
docs/data/data-grid/column-menu/column-menu.md Outdated Show resolved Hide resolved
docs/data/data-grid/column-menu/column-menu.md Outdated Show resolved Hide resolved
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Awesome!

packages/grid/x-data-grid/src/locales/itIT.ts Outdated Show resolved Hide resolved
@@ -10,13 +10,15 @@
{ "name": "CursorCoordinates", "kind": "Interface" },
{ "name": "daDK", "kind": "Variable" },
{ "name": "DATA_GRID_DEFAULT_SLOTS_COMPONENTS", "kind": "Variable" },
{ "name": "DATA_GRID_PRO_DEFAULT_SLOTS_COMPONENTS", "kind": "Variable" },
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to export these?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, we don't need these extra exports, removed them in 4f488b6

@gerdadesign
Copy link
Member

gerdadesign commented Dec 22, 2022

This is shaping up really well!

Just noticing two things:

  1. In the docs under Custom menu component. Is there extra padding added? The menu items no longer appear full-width (this is particularly noticeable on hover). Should there also be a divider between the end of the default menu options and the added custom one?

Screen Shot 2022-12-22 at 10 32 24 AM

  1. In dark mode, the icons look like they are at 100% white. Material Design has icon color guidelines listed for dark mode as Active + Unfocused @ 70% opacity; Focused @ 100%.

@MBilalShafi
Copy link
Member Author

MBilalShafi commented Dec 22, 2022

The menu items no longer appear full-width (this is particularly noticeable on hover).

You are right, this demo doesn't follow the default styling guidelines as the purpose is to depict the customizability possible with the column menu. Though the padding did look a bit off, I adjusted it, thanks for pointing out.

In dark mode, the icons look like they are at 100% white. Material Design has icon color guidelines listed for dark mode as Active + Unfocused @ 70% opacity; Focused @ 100%

I checked, the icons use the same colors as in material-ui <Menu /> component, you can verify by comparing current column menu icons colors and the colors in icons demo for material-ui menu component. (Please let me know if you find some discrepancies)
May be we should update in Menu component itself if it doesn't follow material design guidelines you referred to. 🤔

@gerdadesign
Copy link
Member

gerdadesign commented Dec 23, 2022

I checked, the icons use the same colors as in material-ui <Menu /> component, you can verify by comparing current column menu icons colors and the colors in icons demo for material-ui menu component. (Please let me know if you find some discrepancies)
May be we should update in Menu component itself if it doesn't follow material design guidelines you referred to. 🤔

Thanks for calling this out! It is in the Material UI Menu component. I opened a new issue: mui/material-ui#35589 about it.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Nice UX improvement 👍

function GridColumnMenuContainer(props: GridColumnMenuProps, ref) {
const { hideMenu, currentColumn, open, id, labelledby, className, children, ...other } = props;
const StyledMenuList = styled(MenuList)(() => ({
minWidth: 248,
Copy link
Member

@oliviertassinari oliviertassinari Dec 31, 2022

Choose a reason for hiding this comment

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

I wonder if it's not a bit too much width on mobile. The column menu feels a bit big. Hiding more information on the screen than might be necessary.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 21, 2023

I might have found design opportunities in the aggregation introduced in this PR. I have found them while I was reviewing the changes in https://mui.com/blog/mui-x-v6/. Should we create issues for them? cc @gerdadesign @MBilalShafi

  1. The hover style feels confusing. I would expect a hover style to communicate that I can click, but clicking does nothing:

Screenshot 2023-03-21 at 16 39 00

https://deploy-preview-6619--material-ui-x.netlify.app/x/react-data-grid/aggregation/

How about we remove the hover?

  1. The scrollbar jumps when I open the aggregation:
Screen.Recording.2023-03-21.at.17.06.01.mov

https://deploy-preview-6619--material-ui-x.netlify.app/x/react-data-grid/aggregation/#filtering

@MBilalShafi
Copy link
Member Author

MBilalShafi commented Mar 31, 2023

The column menu feels a bit big. Hiding more information on the screen than might be necessary.

I think the same will be the experience on mobile with aggregation/grouping items shown even if the width constraint is not applied, but we can save some space on Community/Pro versions by adding a media query. 👍

How about we remove the hover?

That makes sense, to improve the user's expectation until we propose a better UX for aggregation item (a dedicated panel or part of some other panel etc)

The scrollbar jumps when I open the aggregation

I think it's because of an issue with <MUISelect /> as it adds some hacky CSS to body when the menu opens up.

What we can possibly do:

  1. Use the SelectUnstyled and style it accordingly
  2. Use native HTML <select />
  3. ✅ Use native prop in MUI Select

I feel the third option is a sweet spot.

  1. We use the same configuration in the Filter panel
  2. It will resolve the flicker issue and also keep the TextInput component's design the same without additional styling

So maybe for now it makes sense to go for possibility no.3

Have tried to apply the above-mentioned changes in #8424

cc @gerdadesign

@oliviertassinari oliviertassinari added design This is about UI or UX design, please involve a designer and removed design: ux labels Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer new feature New feature or request plan: Premium Impact at least one Premium user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Grouped column menu shows redundant Divider [data grid] Improve column menu
8 participants