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

[POC] MenuButton - Option 1 implementation #35671

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 160 additions & 0 deletions docs/pages/experiments/base/menu.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
import * as React from 'react';
import MenuUnstyled from '@mui/base/MenuUnstyled';
import MenuItemUnstyled, { menuItemUnstyledClasses } from '@mui/base/MenuItemUnstyled';
import PopperUnstyled from '@mui/base/PopperUnstyled';
import { styled } from '@mui/system';
import MenuButtonUnstyled from '@mui/base/MenuUnstyled/MenuButtonUnstyled';

const blue = {
100: '#DAECFF',
200: '#99CCF3',
400: '#3399FF',
500: '#007FFF',
600: '#0072E5',
900: '#003A75',
};

const grey = {
50: '#f6f8fa',
100: '#eaeef2',
200: '#d0d7de',
300: '#afb8c1',
400: '#8c959f',
500: '#6e7781',
600: '#57606a',
700: '#424a53',
800: '#32383f',
900: '#24292f',
};

const StyledListbox = styled('ul')(
({ theme }) => `
font-family: IBM Plex Sans, sans-serif;
font-size: 0.875rem;
box-sizing: border-box;
padding: 6px;
margin: 12px 0;
min-width: 200px;
border-radius: 12px;
overflow: auto;
outline: 0px;
background: ${theme.palette.mode === 'dark' ? grey[900] : '#fff'};
border: 1px solid ${theme.palette.mode === 'dark' ? grey[700] : grey[200]};
color: ${theme.palette.mode === 'dark' ? grey[300] : grey[900]};
box-shadow: 0px 4px 30px ${theme.palette.mode === 'dark' ? grey[900] : grey[200]};
`,
);

const StyledMenuItem = styled(MenuItemUnstyled)(
Copy link
Member

@oliviertassinari oliviertassinari Jan 23, 2023

Choose a reason for hiding this comment

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

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.

({ theme }) => `
list-style: none;
padding: 8px;
border-radius: 8px;
cursor: default;
user-select: none;

&:last-of-type {
border-bottom: none;
}

&.${menuItemUnstyledClasses.focusVisible} {
outline: 3px solid ${theme.palette.mode === 'dark' ? blue[600] : blue[200]};
background-color: ${theme.palette.mode === 'dark' ? grey[800] : grey[100]};
color: ${theme.palette.mode === 'dark' ? grey[300] : grey[900]};
}

&.${menuItemUnstyledClasses.disabled} {
color: ${theme.palette.mode === 'dark' ? grey[700] : grey[400]};
}

&:hover:not(.${menuItemUnstyledClasses.disabled}) {
background-color: ${theme.palette.mode === 'dark' ? grey[800] : grey[100]};
color: ${theme.palette.mode === 'dark' ? grey[300] : grey[900]};
}
`,
);

const MenuButton = styled('button')(
Copy link
Member

@oliviertassinari oliviertassinari Jan 23, 2023

Choose a reason for hiding this comment

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

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')(

({ theme }) => `
font-family: IBM Plex Sans, sans-serif;
font-size: 0.875rem;
box-sizing: border-box;
min-height: calc(1.5em + 22px);
border-radius: 12px;
padding: 12px 16px;
line-height: 1.5;
background: ${theme.palette.mode === 'dark' ? grey[900] : '#fff'};
border: 1px solid ${theme.palette.mode === 'dark' ? grey[700] : grey[200]};
color: ${theme.palette.mode === 'dark' ? grey[300] : grey[900]};

transition-property: all;
transition-timing-function: cubic-bezier(0.4, 0, 0.2, 1);
transition-duration: 120ms;

&:hover {
background: ${theme.palette.mode === 'dark' ? grey[800] : grey[50]};
border-color: ${theme.palette.mode === 'dark' ? grey[600] : grey[300]};
}

&:focus {
border-color: ${blue[400]};
outline: 3px solid ${theme.palette.mode === 'dark' ? blue[500] : blue[200]};
}
`,
);

const Popper = styled(PopperUnstyled)`
z-index: 1;
`;

const Page = styled('div')(`
max-width: 800px;
min-height: calc(100vh - 40px);
box-sizing: border-box;
margin: 20px auto;
padding: 20px;
background: ${grey[100]};
border-radius: 4px;
`);

export default function UnstyledMenuIntroduction() {
const [isOpen, setOpen] = React.useState(false);

const close = () => {
setOpen(false);
};

const handleOpenChange = (event: React.SyntheticEvent<HTMLButtonElement>, open: boolean) => {
setOpen(open);
};

const createHandleMenuClick = (menuItem: string) => {
return () => {
// eslint-disable-next-line no-console
console.log(`Clicked on ${menuItem}`);
close();
};
};

return (
<Page>
<MenuButtonUnstyled
slots={{ root: MenuButton }}
label="My account"
Comment on lines +142 to +143
Copy link
Member

@oliviertassinari oliviertassinari Jan 9, 2023

Choose a reason for hiding this comment

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

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>}

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@oliviertassinari oliviertassinari Jan 23, 2023

Choose a reason for hiding this comment

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

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.

open={isOpen}
onOpenChange={handleOpenChange}
>
<MenuUnstyled
slots={{ root: Popper, listbox: StyledListbox }}
slotProps={{ root: { placement: 'bottom-start' }, listbox: { id: 'simple-menu' } }}
>
<StyledMenuItem onClick={createHandleMenuClick('Profile')}>Profile</StyledMenuItem>
<StyledMenuItem onClick={createHandleMenuClick('My account')}>
Language settings
</StyledMenuItem>
<StyledMenuItem onClick={createHandleMenuClick('Log out')}>Log out</StyledMenuItem>
</MenuUnstyled>
</MenuButtonUnstyled>
</Page>
);
}
111 changes: 111 additions & 0 deletions packages/mui-base/src/MenuUnstyled/MenuButtonUnstyled.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import * as React from 'react';
import {
unstable_useForkRef as useForkRef,
unstable_useControlled as useControlled,
} from '@mui/utils';
import { useSlotProps } from '../utils';

export interface MenuButtonProps {
className?: string;
label?: string;
open?: boolean;
onOpenChange?: (
event: React.MouseEvent<HTMLButtonElement> | React.KeyboardEvent<HTMLButtonElement>,
open: boolean,
) => void;
defaultOpen?: boolean;
children?: React.ReactNode;
slots?: {
root?: React.ElementType;
};
slotProps?: {
root?: React.HTMLAttributes<HTMLButtonElement>;
};
}

interface MenuTriggerContextValue {
open: boolean;
buttonRef: React.RefObject<HTMLButtonElement>;
}

export const MenuTriggerContext = React.createContext<MenuTriggerContextValue | null>(null);

const MenuButton = React.forwardRef(function MenuButton(
props: MenuButtonProps,
ref: React.ForwardedRef<HTMLButtonElement>,
) {
const {
children,
label,
open: openProp,
defaultOpen,
onOpenChange,
slots = {},
slotProps = {},
...other
} = props;

const [isOpen, setOpen] = useControlled({
controlled: openProp,
default: defaultOpen,
name: 'MenuButton',
});

const buttonRef = React.useRef<HTMLButtonElement>(null);
const handleRef = useForkRef(buttonRef, ref);

const contextValue: MenuTriggerContextValue = React.useMemo(
() => ({ open: isOpen, buttonRef }),
[isOpen, buttonRef],
);

const handleClick = React.useCallback(
(event: React.MouseEvent<HTMLButtonElement>) => {
setOpen((open) => {
return !open;
});

onOpenChange?.(event, !isOpen);
},
[isOpen, setOpen, onOpenChange],
);

const handleKeyDown = React.useCallback(
(event: React.KeyboardEvent<HTMLButtonElement>) => {
if (event.key === 'ArrowDown') {
event.preventDefault();
setOpen(true);
onOpenChange?.(event, true);
}
},
[setOpen, onOpenChange],
);

React.useEffect(() => {
if (!isOpen) {
buttonRef.current?.focus();
}
}, [isOpen]);

const Root = slots.root || 'button';
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@siriwatknp siriwatknp Jan 12, 2023

Choose a reason for hiding this comment

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

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.

const rootProps = useSlotProps({
elementType: Root,
externalForwardedProps: other,
externalSlotProps: slotProps.root,
additionalProps: {
ref: handleRef,
onClick: handleClick,
onKeyDown: handleKeyDown,
},
ownerState: { ...props, open: isOpen },
});

return (
<MenuTriggerContext.Provider value={contextValue}>
<Root {...rootProps}>{label}</Root>
{children}
Copy link
Member

@siriwatknp siriwatknp Jan 3, 2023

Choose a reason for hiding this comment

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

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' } }}
  >

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for using context

</MenuTriggerContext.Provider>
);
});

export default MenuButton;
14 changes: 12 additions & 2 deletions packages/mui-base/src/MenuUnstyled/MenuUnstyled.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import useMenu from './useMenu';
import composeClasses from '../composeClasses';
import PopperUnstyled from '../PopperUnstyled';
import useSlotProps from '../utils/useSlotProps';
import { MenuTriggerContext } from './MenuButtonUnstyled';

function getUtilityClasses(ownerState: MenuUnstyledOwnerState) {
const { open } = ownerState;
Expand All @@ -39,18 +40,27 @@ const MenuUnstyled = React.forwardRef(function MenuUnstyled<
>(props: MenuUnstyledProps<BaseComponentType>, forwardedRef: React.Ref<any>) {
const {
actions,
anchorEl,
anchorEl: anchorElProp,
children,
component,
keepMounted = false,
listboxId,
onClose,
open = false,
open: openProp = false,
slotProps = {},
slots = {},
...other
} = props;

let open = openProp;
let anchorEl = anchorElProp;

const triggerContext = React.useContext(MenuTriggerContext);
if (triggerContext) {
open = triggerContext.open;
anchorEl = triggerContext.buttonRef.current;
}

const {
registerItem,
unregisterItem,
Expand Down