[Menu][material-next] Use useMenu hook #38934

mnajdova · 2023-09-12T08:09:18Z

Part of #39521
Playground: https://deploy-preview-38934--material-ui.netlify.app/experiments/material-next/menu/

Things done in the PR:

converted to TypeScript
added usage of Base UI's useMenu, useDropdown & useMenuItem hooks
backported the old API that allows using the legacy API (Using the open prop instead of Dropdown context)

Things to be implemented (will require Base UI API changes, so I would leave them for a separate PR):

autoFocus disabling - I will try to find use-cases for this, I don't understand at this point what may be the use-case of having an opened menu with no item focused
variant - selectedMenu (in Base UI at this point, the menu items has no notion of selected, this will mean that we will need to change it to work similar as the select's option, I wonder if this is something that is worth implementing on Base UI's side, but it makes sense in my opinion... cc @michaldudak)

mui-bot · 2023-09-12T08:12:20Z

Netlify deploy preview

https://deploy-preview-38934--material-ui.netlify.app/

@mui/material-next: parsed: +3.50% , gzip: +4.07%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 33bd927

… component

mnajdova · 2023-09-27T13:24:27Z

packages/mui-base/src/useMenu/useMenu.ts

+    listboxRef: listboxRefProp,
+    onItemsChange,
+    id: idParam,
+    disabledItemsFocusable = true,


This option was needed so that we can keep a backward compatible API on Material UI, by default it works as before.

Same applies for the disableListWrap prop below.

mnajdova · 2023-09-27T13:25:08Z

packages/mui-base/src/useMenu/useMenu.ts

+    onItemsChange,
+    id: idParam,
+    disabledItemsFocusable = true,
+    open: openParameter,


With this, I kind of made the hook to work without context too, I know we had a PR to make the prop required, but I don't see a better way in order to support both APIs. I am curious as to what are @michaldudak thoughts on this.

In #39284 I'm also adding the anchor prop to the menu back, so this may suggest that the solution we chose in #32088 wasn't the optimal one.

Anyway, now the question here also is if we're adding this prop back only to support the existing Material UI-style API, or is there a real need for it? If the former, maybe we shouldn't have spent so much time on the new implementation of Base UI, and should have just extracted the unstyled bits from the Material UI component without changing the API. I've always seen Base UI as a fresh start without the strong requirement to have a 100% compatible API with Material UI.

All these aside, I'm wondering if it would be possible to use both the useMenu and useDropdown in the Material Next's Menu (so that the open/close state is handled by the dropdown, while the list navigation is handled by the menu).

All these aside, I'm wondering if it would be possible to use both the useMenu and useDropdown in the Material Next's Menu (so that the open/close state is handled by the dropdown, while the list navigation is handled by the menu).

I don't think this is the optimal solution, especially considering that most of the UI libraries would like to offer both controlled and non-controlled open state for the component, which means all of them would need to use both hooks - if this can be replaced with just an additional argument in the useMenu hook, why wouldn't we do it.

Anyway, now the question here also is if we're adding this prop back only to support the existing Material UI-style API, or is there a real need for it?

In my opinion, this would benefit Joy UI too. Considering this, it's likely going to benefit other libraries that would depend on Base UI.

If the former, maybe we shouldn't have spent so much time on the new implementation of Base UI, and should have just extracted the unstyled bits from the Material UI component without changing the API. I've always seen Base UI as a fresh start without the strong requirement to have a 100% compatible API with Material UI.

Material UI is just one example of a UI library that already has an opinion on how things should work - this is why by using Base UI in it we discover things that can be improved. So far, Joy UI is adapting whatever Base UI has, which does not allow us to test a real-world use case most of the time. What I mean is, I don't think @siriwatknp would be opposed to these changes, but he won't necessarily push you to do them :D I am happy to try to refactor the component to use the useDropdown inside it too, but is that what we want to advertise users of the library to do? If yes, we should also add a demo about it on the Base UI's Menu page.

I agree that we don't know from real-world usage which pattern would be the best. But also, being "the best" could mean something else to different people, so this can be hard to measure. Surely, one option is to provide more ways of doing things, but this comes with a price - more APIs to support and possibly confused users not knowing which API to choose (and perhaps some even more confused, who mix different APIs together). So I'd be in favor of having one API and teaching developers how to use it rather than having many and allowing everyone to choose. This doesn't mean it must be the existing API with a separate Dropdown and Menu - maybe it's Material UI that got it right.

For now let's try using both useDropdown and useMenu in Material UI's Menu to get a feeling about the DX and we can discuss it afterward.

Here is the commit for using both hooks 1d6e378. The downside is that you need to create a wrapper component so that you can provide the dropdown context in the parent of the one that uses the useMenu hook.. Unless you had a different thought on how the two hooks can be combined. Seeing the changes in the Menu.js file, to me it is kind of ridiculous to ask people to do this, if we can easily support providing the open param. However, we can always add this as an additional API afterwards, so happy to try to see what will be the feedback.

Thanks for trying it out!

The downside is that you need to create a wrapper component so that you can provide the dropdown context in the parent of the one that uses the useMenu hook.

Yup, that's unfortunately necessary if you want to keep the existing API.

Seeing the changes in the Menu.js file, to me it is kind of ridiculous to ask people to do this, if we can easily support providing the open param

Let's consider we expose the open parameter and not require the Dropdown to be present. Wouldn't this require developers to handle things like restoring focus on the menu button when a menu is closed or setting up aria-controls and aria-labelledby attributes on their own?

Wouldn't this require developers to handle things like restoring focus on the menu button when a menu is closed or setting up aria-controls and aria-labelledby attributes on their own?

You are right, this would not be a regression for Material UI, as it worked like that before, but it may be surprising for other people trying the hook... We can try to go as is for now and consider adding the API later if we see more people wanting to use the menu as a standalone component - I can't think of valid use-cases apart from the selfish backporting API for Material UI :)

I will update the PR to make it closer to mergable state then without changing the Base UI API further then just adding the disabledItemsFocusable option that was missing.

mnajdova · 2023-09-27T13:27:51Z

packages/mui-material-next/src/Menu/Menu.test.js

+  });
+
+  // TODO: This test is failing
+  // it('should open during the initial mount', () => {


@michaldudak this test is failing for some reason. From what I could see, the difference is that this MenuItem is not using the useMenuItem hook. I am not sure if this is the cause for the fail tough.

At first glance it seems that the MenuItem doesn't get the autoFocus prop from anywhere.

DiegoAndai · 2023-10-03T15:45:23Z

@mnajdova, would it be possible to open a separate PR to copy the Menu files to material-next and merge that first? That way, the diff will be much easier to review, especially with a complex refactor like this one.

mnajdova · 2023-10-04T09:21:31Z

@mnajdova, would it be possible to open a separate PR to copy the Menu files to material-next and merge that first? That way, the diff will be much easier to review, especially with a complex refactor like this one.

Right, I am doing it 👍 So simple, yet so smart strategy for uncovering the diffs :D

DiegoAndai

Hey! left some questions/comments 😊

packages/mui-base/src/useMenu/useMenu.ts

DiegoAndai · 2023-10-18T17:39:29Z

packages/mui-material-next/src/Menu/Menu.test.js

+    act(() => {
+      screen.getByRole('menu').focus();
+    });


Was this required because of autoFocus? Let's add a TODO to remove it if that's the case.

packages/mui-material-next/src/Menu/Menu.test.js

packages/mui-material-next/src/MenuItem/MenuItem.js

DiegoAndai · 2023-10-18T18:04:38Z

packages/mui-material-next/src/MenuItem/MenuItem.test.js


 describe('<MenuItem />', () => {
-  const { render } = createRenderer();
+  const { render } = createRenderer({ clock: 'fake' });


Why this change?

Because some tests were failing. I believe it's again related to how Base UI' compound components work, but I may be wrong. I will dig deeper.

Btw, did you notice the same on the Select component? It's a bit strange why the component needs to render twice. Maybe it's because of the Dropdown context :\

Btw, did you notice the same on the Select component? It's a bit strange why the component needs to render twice. Maybe it's because of the Dropdown context :\

I haven't had to change to {clock: 'fake'}

I have noticed extra renderings but haven't dug deep into that yet. I will let you know if I figure something out.

packages/mui-material-next/src/MenuItem/MenuItem.js

mnajdova · 2023-10-19T12:45:16Z

Thanks for the review @DiegoAndai. I will investigate in more detail why the rendering/focusing is different now. I assume it may be related to the Dropdown wrapping the Menu, or how I use the Base UI's hooks. I will request a new review once I have an explanation about this.

packages/mui-material-next/src/Menu/Menu.test.tsx

packages/mui-material-next/src/Menu/Menu.tsx

packages/mui-material-next/src/MenuItem/MenuItem.test.tsx

Signed-off-by: Marija Najdova <mnajdova@gmail.com>

mnajdova · 2023-10-23T09:32:21Z

@DiegoAndai I think we can resolve the focusing issue in the tests separately, I can see that in Base UI too the keyboard tests are focusing the first item always before testing some keyboard event. I've added this things in the issue #39521

DiegoAndai

LGTM 🚀

Great effort to maintain the legacy API 🙌🏼

[Menu][material-next] Use useMenu hook

9e79f41

mnajdova added component: menu This is the name of the generic UI component, not the React module! proof of concept Studying and/or experimenting with a to be validated approach labels Sep 12, 2023

mnajdova added 14 commits September 12, 2023 12:02

Support the legacy API too

deceed9

Make original test work

2fa4a45

Replicate tests when using context

a5ed2c2

fixes

dbae070

sx prop type

1ece3a2

type issues

960d75e

add more examples

67694eb

Merge branch 'master' into material-next/menu-useMenu

c50f9e6

Allow condiguring disabledItemFocusable

3ca9197

allow setting open prop in the useMenu hook; remove usage of MenuList…

9fca08f

… component

Fix conformance tests, skip the rest failing tests

15fd5d2

one lint issue

9a0e4da

fix MenuItem tests

1ecc3d7

enable tests

e271040

DiegoAndai mentioned this pull request Sep 27, 2023

[material-next][Select] Copy Select files from v5 #39188

Merged

1 task

mnajdova commented Sep 27, 2023

View reviewed changes

mnajdova requested review from michaldudak and DiegoAndai September 27, 2023 13:28

docs:api

3cabcc9

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 28, 2023

Merge branch 'master' into material-next/menu-useMenu

8edc2a3

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 10, 2023

michaldudak mentioned this pull request Oct 16, 2023

[Menu][joy] Remove open and onClose props #38354

Closed

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 17, 2023

mnajdova force-pushed the material-next/menu-useMenu branch from 1d44af6 to 1d6e378 Compare October 17, 2023 09:21

mnajdova added 10 commits October 17, 2023 11:22

conver to TS

0bc670a

disableListWrap option added, added examples for missing features

a0e74c4

docs:api

7a854ea

Be explicit about which props need to be handled

3d4cb52

Make it explicit what needs to be handled, fix some small issues

0c656fa

more comments

a112d66

TS exports

81d7ecf

Merge branch 'master' into material-next/menu-useMenu

50c282c

some ci fixes

d69fb02

update tests

918b377

mnajdova marked this pull request as ready for review October 18, 2023 09:06

DiegoAndai reviewed Oct 18, 2023

View reviewed changes

mnajdova added 4 commits October 19, 2023 13:49

Diego's comment

398e708

Change imports where applicable, convert Menu test to TS

d01fcde

Convert MenuItem to TS

a3df88c

Add comments for replacing components in v6

9dd2160

mnajdova commented Oct 23, 2023

View reviewed changes

mnajdova added 4 commits October 23, 2023 10:57

Apply suggestions from code review

765395b

Signed-off-by: Marija Najdova <mnajdova@gmail.com>

Fix type issues

5ed0c62

yarn rsc:build

1acce90

fix role prop type

33bd927

DiegoAndai approved these changes Oct 23, 2023

View reviewed changes

mnajdova merged commit c82e4a8 into mui:master Oct 24, 2023
22 checks passed

mnajdova mentioned this pull request Oct 24, 2023

[Menu][material-next] Add Material You component #39521

Open

14 tasks

sai6855 mentioned this pull request Dec 30, 2023

[List][material-next] Add all List* components #40368

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Menu][material-next] Use useMenu hook #38934

[Menu][material-next] Use useMenu hook #38934

mnajdova commented Sep 12, 2023 •

edited

mui-bot commented Sep 12, 2023 •

edited

mnajdova Sep 27, 2023

mnajdova Oct 17, 2023

mnajdova Sep 27, 2023

michaldudak Oct 3, 2023

mnajdova Oct 4, 2023

michaldudak Oct 6, 2023

mnajdova Oct 10, 2023

michaldudak Oct 12, 2023

mnajdova Oct 13, 2023

mnajdova Sep 27, 2023

michaldudak Oct 3, 2023

DiegoAndai commented Oct 3, 2023

mnajdova commented Oct 4, 2023

DiegoAndai left a comment

DiegoAndai Oct 18, 2023

DiegoAndai Oct 18, 2023

mnajdova Oct 19, 2023

DiegoAndai Oct 19, 2023

mnajdova commented Oct 19, 2023

mnajdova commented Oct 23, 2023

DiegoAndai left a comment

[Menu][material-next] Use useMenu hook #38934

[Menu][material-next] Use useMenu hook #38934

Conversation

mnajdova commented Sep 12, 2023 • edited

mui-bot commented Sep 12, 2023 • edited

Netlify deploy preview

Bundle size report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoAndai commented Oct 3, 2023

mnajdova commented Oct 4, 2023

DiegoAndai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Oct 19, 2023

mnajdova commented Oct 23, 2023

DiegoAndai left a comment

Choose a reason for hiding this comment

mnajdova commented Sep 12, 2023 •

edited

mui-bot commented Sep 12, 2023 •

edited