-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[MenuButton][base] Create the MenuButtonUnstyled component #32088
Comments
@michaldudak I am about to start the Menu for Joy UI as well. What do you think about using // The usage will be like this:
<MenuUnstyled button={<ButtonUnstyled>trigger</ButtonUnstyled>}>
...
</MenuUnstyled> |
Why not follow the pattern we've got elsewhere with Also, I was thinking about encapsulating all the menuButton related logic in a separate component instead of moving it in the Menu. This way we could have a standalone MenuButton component: <MenuButton label="trigger">
<MenuUnstyled>
...
</MenuUnstyled>
</MenuButton> What do you think? |
I like having I propose using a <MenuButton popup={<Menu>...</Menu>}>
Trigger
</MenuButton> By default, the MenuButton renders a const IconButton = styled('button')(...)
<MenuButton component={IconButton} popup={...}>
<Icon />
</MenuButton> This MenuButton could also be used with a Dialog: <MenuButton popup={<Dialog>...</Dialog>}>
Open
</MenuButton> What do you think? Implementation// MenuButton
return (
<React.Fragment>
<button>...</button>
<Context.Provider value={dataForTheMenu}>{popup}</Context.Provider>
</React.Fragment>
) @michaldudak Any thoughts on my proposal? |
Sorry, it got lost among other notifications :/
One concern I have with this approach is that having a "large" JSX within a prop doesn't look well, and it's harder to read. Let's consider the following: <MenuButton popup={(
<Menu>
<MenuItem>First</MenuItem>
<MenuItem>Second</MenuItem>
<MenuItem>Third</MenuItem>
</Menu>})>
Trigger
</MenuButton> it feels slightly less readable and harder to write (because of nested parenthesis) than <MenuButton label="Trigger">
<Menu>
<MenuItem>First</MenuItem>
<MenuItem>Second</MenuItem>
<MenuItem>Third</MenuItem>
</Menu>
</MenuButton>
This looks great.
That's good, however, keep in mind that MenuButton and Menu need a "special connection" ;) Having written this, another idea of composition came to my mind - a controlled menu button const [open, setOpen] = useState();
<MenuButton popupRef={menuRef} onPopupVisibleChange={setOpen}>Trigger</MenuButton>
<Menu ref={menuRef} open={open}>
...
</Menu> This would decouple definitions of menu and menubutton, but require more work from developers (it won't just work upon placing it on the page). |
I think it is better than confusion compare to: <MenuButton label="Trigger">
<Menu>...</Menu>
</MenuButton> In JSX, this means 2: The other way is to provide a <MenuProvider>
<MenuButton>trigger</MenuButton>
<Menu>...</Menu>
</MenuProvider> 3: Or back to my first proposal, add a // The usage will be like this:
<Menu button={<Button>trigger</Button>}>
...
</Menu>
Fair enough, let's keep the
🤔 I rather not go this way. |
Another idea - how about including the button in the Menu component itself (both Unstyled and Joy)? It would remove the confusion and make things simpler for users. For cases where a button is not required (like a context menu), Then we would have: <Menu label="Trigger">
<MenuItem>...</MenuItem>
...
</Menu> or <Menu components={{ Button: null }}>
... |
But then what is the root slot? the button or the popper? <Menu label="Trigger" data-testid="">
</Menu>
// becomes
<button data-testid="">...</button>
<Popper>...</Popper> I lean toward having the button being root to have a similar experience to the Select.
Using button as a root slot would not work well 🥲. Overall, I still favor MenuButton as a separate component (I might extract Select into |
Perhaps the root slot could be a Fragment by default. I've taken a look at what the other libraries are doing. Chakra, Mantine, and Radix (with Select, as they don't have the menu yet) use the context approach: <Menu>
<MenuButton />
<MenuList>
<MenuItem />
</MenuList>
</Menu> React Spectrum has another solution: <MenuButton>
<button />
<Menu>
...
</Menu>
</MenuButton> and FluentUI uses a completely different approach: <Button menuProps={…} /> |
<Menu>
<MenuButton />
<MenuList>
<MenuItem />
</MenuList>
</Menu> This looks most intuitive API to me. And, in addition, we can also support it with the <Menu
components={{
Button: {},
List: {}.
}}
/> |
@mnajdova @michaldudak I think the resulting API should weigh in:
Here is the comparison for all options: Option 1:encapsulating all the menuButton related logic in a separate component instead of moving it in the Menu. This way we could have a standalone MenuButton component Playground: #35671 <MenuButton label="trigger">
<MenuUnstyled>
...
</MenuUnstyled>
</MenuButton> Cons
Option 2:Playground: #35719 Having <MenuButton popup={(
<Menu>
<MenuItem>First</MenuItem>
<MenuItem>Second</MenuItem>
<MenuItem>Third</MenuItem>
</Menu>})>
Trigger
</MenuButton> Cons
Option 3:Playground: #35778 <Menu label="Trigger">
<MenuItem>...</MenuItem>
...
</Menu> Cons
Option 4:Similar to Chakra UI, This option is similar to Option 5, please use next playground to evaluate it. <Menu>
<MenuButton />
<MenuList>
<MenuItem />
</MenuList>
</Menu> Cons:
Option 5:Quite similar to option 4 but uses a provider name so that it does not break Material UI's Playground: #35782 <MenuProvider>
<MenuButton />
<Menu>
<MenuItem />
</Menu>
</MenuProvider> Cons:
Option 6:use hook, const { getButtonProps, getMenuProps } = usePopup();
<Button {...getButtonProps()}>
<Menu {...getMenuProps()}>...</Menu> Cons
Option 7:Same as #9893 final proposal which uses Playground: #35784 <Menu button={<Button>Open</Button>}>
<MenuItem>...</MenuItem>
</Menu> Even though I think option 4 is intuitive but I guess it would be a breaking change for Material UI (if yes, I rather go with either option 2, 5, or 6 instead) |
@oliviertassinari @danilo-leal @samuelsycamore feel free to jump into the discussion! |
As for Option 4 and your worry about a breaking change - MenuUnstyled has already departed from what was in Material UI. Its implementation was completely rewritten, so we have to expect some breaking changes anyway. |
+1 on Option 4 once again :D |
Just for the record, as we've discussed in one of the meetings - @siriwatknp is going to prepare a POC in Joy, and once we consider the API good enough, we will create a proper implementation in MUI Base and base the Joy version on it. |
From what I understand, this problem has been discussed a couple of times over the history of the project, e.g. #9893. See the difference options explored on that issue. |
After reading #9893 a couple of times, I would like everyone to reconsider the candidate options again. The discussion of the issue consists of 2 topics, the popper behavior, and the menu button. I will focus only on the menu button. The main thing @ryancogswell brought up that triggered me to reconsider is the breaking changes. This is the API that he proposed (same as my first proposal): <Menu button={<Button>Open Menu</Button>} variant="dropdown">
<MenuItem>Item 1</MenuItem>
<MenuItem>Item 2</MenuItem>
<MenuItem>Item 3</MenuItem>
</Menu> He already summarized all the options in #9893 (comment). The main reason I would like everyone to reconsider is the breaking changes. The MenuUnstyled will cause the behavior breaking changes for Material UI but it does not mean that the MenuButton has to be a breaking change. In terms of usage and customization, the only difference between <Menu button={<Button></Button>}>
<Menu>
<MenuButton></MenuButton>
</Menu> There are no differences in terms of customization because both approaches have an explicit button. However, the Menu as a provider will create a lot of work for us (creator), library maintainers (e.g. material-ui-popper-state), and developers (direct users of Material UI). The breaking changes are not just for Material UI but for Joy UI and MUI Base as well. We will have to rewrite the Menu, create a migration codemod, rewrite docs, and update all the demos. Do we have that much time for this in v6? cc @michaldudak @oliviertassinari @mnajdova Here is the POC version of adding |
I still like the JSX like API better, but considering everything I would be ok if we adopt the prop based API too. This would mean that we can add this in all packages immediately right? |
A few thoughts:
It's close to option 5 but a bit different. Regarding the options, personally, I think that: 5 > 1 > 4 > 3 > 2 |
This is a good point. Having a different component for menus triggered by a button and those without (i.e. context menus) makes sense. I'm for having at least a
Sure, breaking changes aren't welcome, but if we can significantly simplify the API while introducing them, IMO they're worth considering. Let's consider the code from https://mui.com/material-ui/react-menu/#positioned-menu. export default function PositionedMenu() {
const [anchorEl, setAnchorEl] = React.useState<null | HTMLElement>(null);
const open = Boolean(anchorEl);
const handleClick = (event: React.MouseEvent<HTMLElement>) => {
setAnchorEl(event.currentTarget);
};
const handleClose = () => {
setAnchorEl(null);
};
return (
<div>
<Button
id="demo-positioned-button"
aria-controls={open ? 'demo-positioned-menu' : undefined}
aria-haspopup="true"
aria-expanded={open ? 'true' : undefined}
onClick={handleClick}
>
Dashboard
</Button>
<Menu
id="demo-positioned-menu"
aria-labelledby="demo-positioned-button"
anchorEl={anchorEl}
open={open}
onClose={handleClose}
anchorOrigin={{
vertical: 'top',
horizontal: 'left',
}}
transformOrigin={{
vertical: 'top',
horizontal: 'left',
}}
>
<MenuItem onClick={handleClose}>Profile</MenuItem>
<MenuItem onClick={handleClose}>My account</MenuItem>
<MenuItem onClick={handleClose}>Logout</MenuItem>
</Menu>
</div>
);
} could become this: export default function PositionedMenu() {
return (
<MenuButton label="Dashboard">
id="demo-positioned-menu"
anchorOrigin={{
vertical: 'top',
horizontal: 'left',
}}
transformOrigin={{
vertical: 'top',
horizontal: 'left',
}}
>
<MenuItem onClick={handleClose}>Profile</MenuItem>
<MenuItem onClick={handleClose}>My account</MenuItem>
<MenuItem onClick={handleClose}>Logout</MenuItem>
</MenuButton>
);
} It's much more simple to use. It's also familiar and consistent as it uses patterns similar to Select. A solution like Option 5, with a separate provider, will be less straightforward. Having four different components instead of two means more writing and being harder to remember.
This is the API that he proposed (same as my first proposal): <Menu button={<Button>Open Menu</Button>} variant="dropdown">
<MenuItem>Item 1</MenuItem>
<MenuItem>Item 2</MenuItem>
<MenuItem>Item 3</MenuItem>
</Menu> This is a similar approach but doesn't use the customization patterns we have today in the library. We settled on using <MenuButton label="Open Menu" slots={{ button: Button }}>
<MenuItem>Item 1</MenuItem>
<MenuItem>Item 2</MenuItem>
<MenuItem>Item 3</MenuItem>
</MenuButton> |
@michaldudak This point reminds me of a discussion with @joserodolfofreitas about how high-level the API of the data grid and date picker should be. I would like to defend the following interpretation of reality: boilerplate for our use case might not matter:
From my perspective, the slots API is designed to solve the advanced customization problem, it's not designed for a default component use case. In the problem that we are considering, I think that the extra flexibility of React elements that can be manipulated in the render function (over React components/slots) is important. |
I understand that the proposal from @michaldudak leans toward Select component but honestly, my first intention is to make the Select lean toward Menu in Joy UI 😅 Seems like:
Or should MUI Base and Joy UI have different APIs because they target different audiences? |
Perhaps we should ask the developers what API they'll prefer to use. |
@michaldudak If the use case considered is a niche one, I think that it should be done by controlling the If the use case is common, I think that aria attributes could cover it, like Jun's
Maybe for this component, but there are many cases where both make sense. e.g. we expose a TextField and FormControl, Inputs, exactly like Reach that has two Slider APIs: https://reach.tech/slider. So I think that the concept of slot will never be obsolete. |
I created playgrounds for various options listed in #32088 (comment):
Please check out the code sandboxes linked from each of these PRs and evaluate the solution. Take note of how easy it is to create a basic menu, one with added slot props, and an extensively customized one. I'm curious to see what you think about these options. |
One aspect that could help decide is how the migration looks like for existing Material UI projects: Let's use a simple snippet from the docs const [anchorEl, setAnchorEl] = React.useState<null | HTMLElement>(null);
const open = Boolean(anchorEl);
const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {
setAnchorEl(event.currentTarget);
};
const handleClose = () => {
setAnchorEl(null);
};
<Button
id="basic-button"
aria-controls={open ? 'basic-menu' : undefined}
aria-haspopup="true"
aria-expanded={open ? 'true' : undefined}
onClick={handleClick}
>
Dashboard
</Button>
<Menu
id="basic-menu"
anchorEl={anchorEl}
open={open}
onClose={handleClose}
MenuListProps={{
'aria-labelledby': 'basic-button',
}}
>
<MenuItem onClick={handleClose}>Profile</MenuItem>
…
</Menu> Option 1+<MenuButton id="basic-button" label="Dashboard" slots={{ root: Button }}>
+ <Menu
- id="basic-menu"
- MenuListProps={{
- 'aria-labelledby': 'basic-button',
- }}
>
<MenuItem>Profile</MenuItem>
…
</Menu>
+</MenuButton> Option 2+<MenuButton
+ id="basic-button"
+ slots={{ root: Button }}
+ popup={
<Menu
- id="basic-menu"
- MenuListProps={{
- 'aria-labelledby': 'basic-button',
- }}
>
<MenuItem>Profile</MenuItem>
…
</Menu>
+ }
+ >
+ Dashboard
+</MenuButton> Option 3<Menu
- id="basic-menu"
- anchorEl={anchorEl}
- open={open}
- onClose={handleClose}
- MenuListProps={{
- 'aria-labelledby': 'basic-button',
- }}
+ label="Dashboard"
+ slots={{ button: Button }}
+ slotProps={{ button: { variant: 'text', color: 'primary' } }}
>
<MenuItem onClick={handleClose}>Profile</MenuItem>
…
</Menu> Option 5I am not sure where the +<MenuProvider>
+ <MenuButton slots={{ root: Button }}>Dashboard</MenuButton>
<Menu
- id="basic-menu"
- anchorEl={anchorEl}
- open={open}
- onClose={handleClose}
- MenuListProps={{
- 'aria-labelledby': 'basic-button',
- }}
>
<MenuItem onClick={handleClose}>Profile</MenuItem>
…
</Menu>
+</MenuProvider> Option 7<Menu
id="basic-menu"
- anchorEl={anchorEl}
- open={open}
- onClose={handleClose}
- MenuListProps={{
- 'aria-labelledby': 'basic-button',
- }}
+ button={<Button>Dashboard</Button>}
>
<MenuItem onClick={handleClose}>Profile</MenuItem>
…
</Menu> |
@siriwatknp Great initiative 👍. I have updated the diffs based on what I understood in the different PRs. The important distinction I made is to provide |
@oliviertassinari with option 5, this is how it looks like in Joy UI: Basicimport { MenuProvider, MenuButton, Menu, MenuItem } from '@mui/joy';
<MenuProvider>
<MenuButton startDecorator={<Icon />}>
Open
</MenuButton>
<Menu>
<MenuItem>
…
</Menu>
</MenuProvider> IconButton or custom buttonProvide the component to the import { IconButton, MenuButton, Menu, MenuItem } from '@mui/joy';
<MenuProvider>
// <MenuButton slots={{ root: CustomButton }}>
<MenuButton slots={{ root: IconButton }}>
<Icon />
</MenuButton>
<Menu>
<MenuItem>
…
</Menu>
</MenuProvider> Button group<ButtonGroup>
<Button>Save</Button>
<MenuProvider>
<MenuButton startDecorator={<Icon />}>
Open
</MenuButton>
<Menu>
<MenuItem>
…
</Menu>
</MenuProvider>
</ButtonGroup> |
After the discussions within the team, we've chosen to implement Option 5. I'm going to start the actual implementation next week. |
After creating demos for #30961 it became clear that leaving implementation of menu buttons to developers would force them to write a lot of code. We can create an abstraction for a button that triggers the appearance of a menu and responds to keyboard input (in a slightly different way than a normal button - pressing up/down arrow keys should also open the menu).
Bonus points for making it work with SelectUnstyled.
Note: make sure that clicking on the button when menu is open does not cause blinking, as it's currently the case in MenuUnstyled demos (see #32661 (comment), point 1)
The text was updated successfully, but these errors were encountered: