Add <Menu.Item>content</Menu.item> syntax

[fzaninotto]

Problem

The <Menu.Item primaryText> syntax isn't intuitive, especially for someone who already knows material-ui, where the menu label is expressed via children.

Solution

Support menu label is expressed via children in addition to primaryText for mobile

import { Menu } from 'react-admin';
import { ListItemIcon, ListItemText, Typography } from '@mui/material';

const CustomMenu = () => (
    <Menu>
        <Menu.Item to="/" leftIcon={<Dashboard />} primaryText="Dashboard" />
        <Menu.Item to="/sales" primaryText="Sales">
            <ListItemIcon>
               <PieChartOutlined />
             </ListItemIcon>
             <ListItemText>Sales</ListItemText>
             <Typography variant="body2" color="text.secondary">
                 ⌘S
             </Typography>
        </Menu.Item>
        <Menu.Item to="/customers" leftIcon={<PeopleOutlined />} primaryText="Customers" />
        <Menu.Item to="/products" leftIcon={<Inventory />} primaryText="Catalog" />
    </Menu>
);

The following is still supported:

const CustomMenu = () => (
    <Menu>
        <Menu.Item to="/" leftIcon={<Dashboard />} primaryText="Dashboard" />
        <Menu.Item to="/sales" leftIcon={<PieChartOutlined />} primaryText="Sales" />
        <Menu.Item to="/customers" leftIcon={<PeopleOutlined />} primaryText="Customers" />
        <Menu.Item to="/products" leftIcon={<Inventory />} primaryText="Catalog" />
    </Menu>
);

[djhi]

We should deprecate primaryText to remove it in v5 IMO

[slax57]

Looks very nice !

Just found a little issue: the Tooltip is still rendered on hover when providing children, and it's empty so it's a bit weird.

[fzaninotto]

Because we have to show the menu label on hover on Mobile, we must always support primaryText, even when passing children. But then we end up with duplicated label, because we have to render both in the expanded menu for backwards compatibility reasons.

The only BC solution I can think of is to deprecate primaryText and replace it with label, and only render label in expanded menu if there is no children. But this is a much larger change:

So we should probably not move forward with this. Anyone willing to use custom menu content can still use their own component instead of <Menu.Item>.

WDYT?

[slax57]

@fzaninotto You're right.

However, I don't understand why you are saying "we have to render both in the expanded menu for backwards compatibility reasons".

Unless I'm mistaken, <Menu.Item> did not accept children in the first place (or rather, ignored them). So why couldn't we implement a logic that says: render the children under <StyledMenuItem> when there are children provided, otherwise render primaryText ; and in any case, render primaryText as the <Tooltip title>?

The following seems to work according to my tests:

    const renderMenuItem = () => {
        return (
            <StyledMenuItem
                className={clsx(className, {
                    [MenuItemLinkClasses.active]: !!match,
                })}
                // @ts-ignore
                component={LinkRef}
                ref={ref}
                tabIndex={0}
                {...rest}
                onClick={handleMenuTap}
            >
                {leftIcon && (
                    <ListItemIcon className={MenuItemLinkClasses.icon}>
                        {leftIcon}
                    </ListItemIcon>
                )}
                {children
                    ? children
                    : typeof primaryText === 'string'
                    ? translate(primaryText, { _: primaryText })
                    : primaryText}
            </StyledMenuItem>
        );
    };

    return open ? (
        renderMenuItem()
    ) : (
        <Tooltip
            title={
                typeof primaryText === 'string'
                    ? translate(primaryText, { _: primaryText })
                    : null
            }
            placement="right"
            {...tooltipProps}
        >
            {renderMenuItem()}
        </Tooltip>
    );

[fzaninotto]

You're right, it works! I've pushed the changes. Switching back to RFR.

[slax57]

Unfortunately this Tooltip displays all the time, not only when the side menu is collapsed. Not sure it's worth it to bother fixing it though, it's really minor...

[fzaninotto]

I saw it. This is indeed minor, and it's on an internal story, so I though we could live with it.