[POC] MenuButton - Option 1 implementation #35671

michaldudak · 2022-12-30T12:51:39Z

DO NOT MERGE

This is a playground for evaluating the API of a MenuButton component.
It implements Option 1 from #32088 (comment)

Playground: https://codesandbox.io/s/menubutton-option-1-byxysy

https://deploy-preview-35671--material-ui.netlify.app/experiments/base/menu/

This is a POC that should give a general idea of how the API can look like. It is by no means a complete implementation. There may be some rough edges or unimplemented features.
Do not analyze the implementation - it's quick and dirty.

mui-bot · 2022-12-30T12:54:21Z

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

Details of bundle changes

Generated by 🚫 dangerJS against d17b252

siriwatknp · 2023-01-03T02:58:15Z

packages/mui-base/src/MenuUnstyled/MenuButtonUnstyled.tsx

+  return (
+    <MenuButtonContext.Provider value={contextValue}>
+      <Root {...rootProps}>{label}</Root>
+      {children}


Shouldn't the MenuButton pass these props (or from context)?

Suggested change

{children}

{React.cloneElement(children, { anchorEl, ref })}

One important reason of having a MenuButton component (in my opinion) is that developers don't have to declare the anchorEl and open at the top of the component. The usage should be like this:

<MenuButtonUnstyled slots={{ root: MenuButton }} label="My account" > <MenuUnstyled slots={{ root: Popper, listbox: StyledListbox }} slotProps={{ root: { placement: 'bottom-start' }, listbox: { id: 'simple-menu' } }} >

cc @oliviertassinari

Having {React.cloneElement(children, { anchorEl, ref })} prevents developers from having intermediate components between MenuButton and Menu (or a Fragment). I'll try to achieve this with a context instead.

+1 for using context

siriwatknp

Looks great! Can you also add a useMenuButton hook?

michaldudak · 2023-01-04T08:04:37Z

I'd like to focus on implementing other options first so we can evaluate them before going deeper into any of them. Ultimately, after we choose a winning option, I'll create both a component and a hook.

oliviertassinari

Otherwise, no real concerns with this direction. it sounds fair 👍

oliviertassinari · 2023-01-09T00:52:21Z

docs/pages/experiments/base/menu.tsx

+        slots={{ root: MenuButton }}
+        label="My account"


The use of slots feels strange. Aren't the slot means to replace existing implementation for customization use cases? In this case, I would imagine that what's rendered as a menu trigger will most of the time be "userland". So I think that someone like this:

Suggested change

slots={{ root: MenuButton }}

label="My account"

trigger={<MenuButton>My account</MenuButton>}

would make more sense. Also notice that it's an element, not a component in this proposal. Why? For the extra flexibility needed when dealing with userland APIs. By "userland", I mean API that is used by the product teams in the normal course of building a product using a design system. This is to be distinguished from the API used by the product infrastructure team, the one building the design system with MUI Base.

Actually, in the demo, I think that this terminology would be clearer like this, to signal it can be any button:

Suggested change

slots={{ root: MenuButton }}

label="My account"

trigger={<Button>My account</Button>}

I would imagine that what's rendered as a menu trigger will most of the time be "userland"

Why do you think developers will want to use something different than button by default? Do you think Menu is different in this regard than, let's say, a Select?

Why do you think developers will want to use something different than button by default? Do you think Menu is different in this regard than, let's say, a Select?

I'm more thinking about the following use cases:

<MenuButtonUnstyled slots={{ root: Button }} label="My account" variant="outlined" size="small" >

I think that at the very least label should be renamed to children:

<MenuButtonUnstyled slots={{ root: Button }} variant="outlined" size="small" > My account </MenuButtonUnstyled>

It could be clearer with this to match the terminology used in Material UI / Joy UI slots={{ root: Button }} -> component. But it's still the same concept so I guess people will learn about it. To me, it's also that it doesn't look very practical to write if it's a common use case.

An even more flexible option (it would use React.cloneElement under the hood) could be:

<MenuTrigger> <Button variant="outlined" size="small"> My account </Button> >

with one downside: more boilerplate, which developers can abstract if they want (easy), but they can choose, we don't decide for them.

mnajdova · 2023-01-11T07:29:20Z

packages/mui-base/src/MenuUnstyled/MenuButtonUnstyled.tsx

+    }
+  }, [isOpen]);
+
+  const Root = slots.root || 'button';


Only thing I am not sure here is the name root. I would imagine the MenuButton to have slots menu and button, or if we want to be more generic menu and trigger. Considering the menu is the children element, I would rename the slot.root to slot.trigger.

Right, it makes sense. With a small caveat - we usually forward extra props to the root slot. I wonder where they should go in this case. I'd say the button slot, but this will have to be well documented.

hmm, isn't it better to keep only the root slot for the MenuButtonUnstyled for consistency?

From my understanding, MUI Base provides slots prop for inserting components (usually with styles). The root refers to the topmost DOM of the component. In this case, the MenuButtonUnstyled has only one DOM (default as button) so I am not sure why it needs menu slot.

Changing it to trigger will make the concept inconsistent and developers have to remember one more thing.

oliviertassinari · 2023-01-23T14:43:06Z

docs/pages/experiments/base/menu.tsx

+  `,
+);
+
+const MenuButton = styled('button')(


I think that it would be better to call it Button to reenforce that developers can directly use the button from their design system:

Suggested change

const MenuButton = styled('button')(

const Button = styled('button')(

oliviertassinari · 2023-01-23T14:47:10Z

docs/pages/experiments/base/menu.tsx

+  `,
+);
+
+const StyledMenuItem = styled(MenuItemUnstyled)(


Maybe we don't need the Styled prefix if the base component has Unstyled in its name? This could be done here and in all the docs:

Suggested change

const StyledMenuItem = styled(MenuItemUnstyled)(

const MenuItem = styled(MenuItemUnstyled)(

The convention could be, anything with Unstyled needs to be styled to be used. The rest is ready to use, like Portal.

[POC] Implement Option 1

3a58b05

michaldudak mentioned this pull request Dec 30, 2022

[MenuButton][base] Create the MenuButtonUnstyled component #32088

Closed

michaldudak added the proof of concept Studying and/or experimenting with a to be validated approach label Jan 2, 2023

siriwatknp reviewed Jan 3, 2023

View reviewed changes

michaldudak added the package: base-ui Specific to @mui/base label Jan 3, 2023

Provide anchorEl with context

d17b252

siriwatknp reviewed Jan 4, 2023

View reviewed changes

oliviertassinari added the component: menu This is the name of the generic UI component, not the React module! label Jan 9, 2023

oliviertassinari reviewed Jan 9, 2023

View reviewed changes

mnajdova reviewed Jan 11, 2023

View reviewed changes

oliviertassinari reviewed Jan 23, 2023

View reviewed changes

michaldudak closed this Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] MenuButton - Option 1 implementation #35671

[POC] MenuButton - Option 1 implementation #35671

michaldudak commented Dec 30, 2022 •

edited

Loading

mui-bot commented Dec 30, 2022 •

edited

Loading

siriwatknp Jan 3, 2023 •

edited

Loading

siriwatknp Jan 3, 2023

michaldudak Jan 3, 2023

mnajdova Jan 11, 2023

siriwatknp left a comment

michaldudak commented Jan 4, 2023

oliviertassinari left a comment

oliviertassinari Jan 9, 2023 •

edited

Loading

michaldudak Jan 11, 2023

oliviertassinari Jan 23, 2023 •

edited

Loading

mnajdova Jan 11, 2023

michaldudak Jan 11, 2023

siriwatknp Jan 12, 2023 •

edited

Loading

oliviertassinari Jan 23, 2023 •

edited

Loading

oliviertassinari Jan 23, 2023 •

edited

Loading

	slots={{ root: MenuButton }}
	label="My account"
	trigger={<MenuButton>My account</MenuButton>}

	slots={{ root: MenuButton }}
	label="My account"
	trigger={<Button>My account</Button>}

	const MenuButton = styled('button')(
	const Button = styled('button')(

	const StyledMenuItem = styled(MenuItemUnstyled)(
	const MenuItem = styled(MenuItemUnstyled)(

[POC] MenuButton - Option 1 implementation #35671

[POC] MenuButton - Option 1 implementation #35671

Conversation

michaldudak commented Dec 30, 2022 • edited Loading

mui-bot commented Dec 30, 2022 • edited Loading

siriwatknp Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

siriwatknp Jan 3, 2023

Choose a reason for hiding this comment

michaldudak Jan 3, 2023

Choose a reason for hiding this comment

mnajdova Jan 11, 2023

Choose a reason for hiding this comment

siriwatknp left a comment

Choose a reason for hiding this comment

michaldudak commented Jan 4, 2023

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

michaldudak Jan 11, 2023

Choose a reason for hiding this comment

oliviertassinari Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

mnajdova Jan 11, 2023

Choose a reason for hiding this comment

michaldudak Jan 11, 2023

Choose a reason for hiding this comment

siriwatknp Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

michaldudak commented Dec 30, 2022 •

edited

Loading

mui-bot commented Dec 30, 2022 •

edited

Loading

siriwatknp Jan 3, 2023 •

edited

Loading

oliviertassinari Jan 9, 2023 •

edited

Loading

oliviertassinari Jan 23, 2023 •

edited

Loading

siriwatknp Jan 12, 2023 •

edited

Loading

oliviertassinari Jan 23, 2023 •

edited

Loading

oliviertassinari Jan 23, 2023 •

edited

Loading