[Menu] Use ButtonBase in MenuItem #26591

siriwatknp · 2021-06-04T07:20:54Z

BREAKING CHANGES

Change the default value of anchorOrigin.vertical to follow the Material Design guidelines. The menu now displays below the anchor instead of on top of it.
This conversation was marked as resolved by siriwatknp
Show conversation
You can restore the previous behavior with:
```
 <Menu
+  anchorOrigin={{
+    vertical: 'top',
+    horizontal: 'left',
+  }}
```
The MenuItem component inherits the ButtonBase component instead of ListItem.
The class names related to "MuiListItem-*" are removed and theming ListItem is no longer affecting MenuItem.
```
-<li className="MuiButtonBase-root MuiMenuItem-root MuiListItem-root">
+<li className="MuiButtonBase-root MuiMenuItem-root">
```

prop listItemClasses is removed, use classes instead.

-<MenuItem listItemClasses={{...}}>
+<MenuItem classes={{...}}>

closes [Menu] Remove MenuItem dependency on ListItem #21587
closes [MenuItem] spacing between ListItemIcon and ListItemText #14891
closes CenteredListItemIcon #17681 (added demo)

Preview: https://deploy-preview-26591--material-ui.netlify.app/components/menus/

Changes

replace ListItem with ButtonBase in MenuItem
add 3 more demos (IconMenu, DenseMenu and AccountMenu)
overrides nested component when compose together to have good looking by default
- ListItemIcon
- ListItemText
- Divider
height of MenuItem

dense xs sm (up)

true 36px 36px

false 48px 36px

ListItem also has 36px with dense: true

The reason I add css to override multiple component because I think this is MenuItem's responsibility to provide good looking by default (follow Material Design). If developers needs highly customisation, they can skip MenuItem and build from ListItemButton instead.

same markup

v4

https://codesandbox.io/s/material-demo-forked-o84t1?file=/demo.tsx

v5

I have followed (at least) the PR section of the contributing guide.

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

…at/list-item-improvement

…Base

…at/list-item-improvement

siriwatknp · 2021-06-11T03:43:02Z

How about converting what we had in v4?

updated. https://deploy-preview-26591--material-ui.netlify.app/components/menus/#positioned-menu

packages/material-ui/src/MenuItem/MenuItem.js

mnajdova

Let's add a breaking changes section in migration-v4.md

siriwatknp · 2021-06-11T15:41:10Z

Let's add a breaking changes section in migration-v4.md

It is added to migration-v4.md in MenuItem section

looks good

oliviertassinari · 2021-06-12T17:24:39Z

@siriwatknp We are off-topic with this discussion but it's still interesting for the future. It makes me think about #9893 that we talked about with @eps1lon at some point. It seems that what you are expecting is the menu button to keep the focus when the popup opens.

Take a look at google docs.

From what I understand, Google Docs implements the menu (AKA menu bar) pattern: https://www.w3.org/TR/wai-aria-practices/#menu. The <Menu> implements the menu button pattern: https://www.w3.org/TR/wai-aria-practices/#menubutton.

why the grey color cover the second menu.

Because when you click to open, the first menu item is focused, if press Enter, it activates this first option. You will find the same behavior here: https://stripe.com/docs/.

docs/src/pages/components/menus/menus.md

docs/src/pages/guides/migration-v4/migration-v4.md

packages/material-ui/src/Menu/Menu.js

packages/material-ui/src/MenuItem/MenuItem.d.ts

mnajdova · 2021-06-14T11:52:43Z

packages/material-ui/src/MenuItem/MenuItem.d.ts

+    /**
+     * The system prop that allows defining system overrides as well as additional CSS styles.
+     */
+    sx?: SxProps<Theme>;


Suggested change

/**

* The system prop that allows defining system overrides as well as additional CSS styles.

*/

sx?: SxProps<Theme>;

Same as previous?

If removed, it is all gone from docs api and proptype. Let's keep it for now.

I think on the docs there is info that it extends some interface, so that should be enough.

But Button, IconButton and ListItemButton also has sx in there d.ts file, should we keep the same for MenuItem? not really sure.

Let’s keep it, but leave this conversation open, so that we can address it later.

Co-authored-by: Marija Najdova <mnajdova@gmail.com>

oliviertassinari

I can't notice anything else 👍

packages/material-ui/src/MenuItem/menuItemClasses.ts

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

mnajdova · 2021-06-16T05:39:05Z

docs/src/pages/guides/migration-v4/migration-v4.md

+
+  ```diff
+  -<li className="MuiButtonBase-root MuiMenuItem-root MuiListItem-root">
+  +<li className="MuiButtonBase-root MuiMenuItem-root">


Let's also add this BC:

And please update the PR description as well :)

mnajdova

One last comment for polishing the breaking changes description, apart from that looks good 👍

eps1lon

Great summary description 👍

Just have some typos

docs/src/pages/components/menus/menus.md

Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>

siriwatknp and others added 30 commits May 25, 2021 15:20

add wip ListItemButton

e4f684d

update lists docs to use ListItemButton

a0d6928

run proptypes and docs

41b248b

fix typo name

ae4db46

minor fix

f047a2c

mark button related prop as deprecated

8fb85ca

fix description

3f46f02

add usage guide to docs

4f385a9

add dense note

83eadf9

Update docs/src/pages/components/lists/lists.md

1678d53

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Merge branch 'next' of https://github.com/mui-org/material-ui into fe…

8941802

…at/list-item-improvement

add action prop and update demos

43a0f75

run proptypes, docs:api, docs:typescript

aeb3eb3

add customized demo and theme component props

4edef7d

update demo

4f931d0

refactor ListItem.d.ts

9c49711

add disablePadding prop

6b1e037

update demo to use disablePadding

7040bcc

follow disablePadding in List

0884aa6

rename action to secondaryAction due to type conflict with Button…

36ff3d1

…Base

minor fix

fce2d50

Merge branch 'next' of https://github.com/mui-org/material-ui into fe…

7613961

…at/list-item-improvement

cleanup

eb95812

Merge branch 'next' of https://github.com/mui-org/material-ui into fe…

c26f233

…at/list-item-improvement

fix types

3ee7f40

use classes from ListItemButton

2db70c3

replace ListItem with ListItemButton

47e9310

update Menu demos

ef9ac53

removed deprecated tests

c80254e

fix spec

aa6f764

bring disableGutters back

fea2336

mnajdova reviewed Jun 11, 2021

View reviewed changes

packages/material-ui/src/MenuItem/MenuItem.js Show resolved Hide resolved

mnajdova reviewed Jun 11, 2021

View reviewed changes

fix migration

dabad8d

oliviertassinari added 3 commits June 12, 2021 19:00

prettier

5f85ac8

polish changelog

e8622f0

use same terminology as in the demo

90a99cc

oliviertassinari added 2 commits June 12, 2021 19:14

fix import

a1efeb8

Merge branch 'next' into HEAD

8a6d04a

siriwatknp requested a review from mnajdova June 14, 2021 04:40

mnajdova reviewed Jun 14, 2021

View reviewed changes

Apply suggestions from code review

a92b817

Co-authored-by: Marija Najdova <mnajdova@gmail.com>

siriwatknp requested review from mnajdova and oliviertassinari June 15, 2021 03:16

oliviertassinari reviewed Jun 15, 2021

View reviewed changes

packages/material-ui/src/MenuItem/menuItemClasses.ts Outdated Show resolved Hide resolved

Update packages/material-ui/src/MenuItem/menuItemClasses.ts

e941a0c

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

mnajdova reviewed Jun 16, 2021

View reviewed changes

mnajdova approved these changes Jun 16, 2021

View reviewed changes

update migration

8b90383

eps1lon approved these changes Jun 16, 2021

View reviewed changes

docs/src/pages/components/menus/menus.md Outdated Show resolved Hide resolved

docs/src/pages/components/menus/menus.md Outdated Show resolved Hide resolved

Apply suggestions from code review

19d0eb8

Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>

siriwatknp merged commit 1406f51 into mui:next Jun 16, 2021

siriwatknp deleted the menu-item-depencency branch June 16, 2021 07:01

siriwatknp mentioned this pull request Jun 23, 2021

v5.0.0 alpha.38 #26906

Merged

1 task

mnajdova mentioned this pull request Jun 28, 2021

TS2322 Spreading declared MenuItemProps on MenuItem fails type checking #16245

Closed

2 tasks

mnajdova mentioned this pull request Dec 21, 2022

[docs] Fix API page for MenuItem to list all valid props #35561

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Menu] Use ButtonBase in MenuItem #26591

[Menu] Use ButtonBase in MenuItem #26591

siriwatknp commented Jun 4, 2021 •

edited

Loading

siriwatknp commented Jun 11, 2021

mnajdova left a comment

siriwatknp commented Jun 11, 2021 •

edited

Loading

oliviertassinari commented Jun 12, 2021 •

edited

Loading

mnajdova Jun 14, 2021

siriwatknp Jun 15, 2021

mnajdova Jun 15, 2021

siriwatknp Jun 15, 2021 •

edited

Loading

mnajdova Jun 15, 2021

oliviertassinari left a comment

mnajdova Jun 16, 2021

mnajdova left a comment

eps1lon left a comment

`dense`	xs	sm (up)
true	36px	36px
false	48px	36px

[Menu] Use ButtonBase in MenuItem #26591

[Menu] Use ButtonBase in MenuItem #26591

Conversation

siriwatknp commented Jun 4, 2021 • edited Loading

v4

v5

siriwatknp commented Jun 11, 2021

mnajdova left a comment

Choose a reason for hiding this comment

siriwatknp commented Jun 11, 2021 • edited Loading

oliviertassinari commented Jun 12, 2021 • edited Loading

mnajdova Jun 14, 2021

Choose a reason for hiding this comment

siriwatknp Jun 15, 2021

Choose a reason for hiding this comment

mnajdova Jun 15, 2021

Choose a reason for hiding this comment

siriwatknp Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

mnajdova Jun 15, 2021

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

mnajdova Jun 16, 2021

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

siriwatknp commented Jun 4, 2021 •

edited

Loading

siriwatknp commented Jun 11, 2021 •

edited

Loading

oliviertassinari commented Jun 12, 2021 •

edited

Loading

siriwatknp Jun 15, 2021 •

edited

Loading