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

feat(menu): deprecate props.mode / always support full capabilities + icons #18153

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -4608,15 +4608,7 @@ Map {
"menuAlignment": Object {
"type": "string",
},
"mode": Object {
"args": Array [
Array [
"full",
"basic",
],
],
"type": "oneOf",
},
"mode": [Function],
"onClose": Object {
"type": "func",
},
Expand Down
50 changes: 0 additions & 50 deletions packages/react/src/components/ComboButton/ComboButton-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,56 +186,6 @@ describe('ComboButton', () => {
).toHaveTextContent(/^Additional action$/);
});

it('warns when MenuItemSelectable is used in children', async () => {
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {});

render(
<ComboButton label="Primary action">
<MenuItemSelectable label="Option" />
</ComboButton>
);

await userEvent.click(screen.getAllByRole('button')[1]);

expect(spy).toHaveBeenCalled();
spy.mockRestore();
});

it('warns when MenuItemRadioGoup is used in children', async () => {
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {});

render(
<ComboButton label="Primary action">
<MenuItemRadioGroup
label="Options"
items={['Option 1', 'Option 2']}
/>
</ComboButton>
);

await userEvent.click(screen.getAllByRole('button')[1]);

expect(spy).toHaveBeenCalled();
spy.mockRestore();
});

it('warns when a nested Menu is used in children', async () => {
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {});

render(
<ComboButton label="Primary action">
<MenuItem label="Submenu">
<MenuItem label="Action" />
</MenuItem>
</ComboButton>
);

await userEvent.click(screen.getAllByRole('button')[1]);

expect(spy).toHaveBeenCalled();
spy.mockRestore();
});

it('supports ellipsis in ComboButton by checking the className', async () => {
render(
<ComboButton label="Primary action super long text to enable ellipsis">
Expand Down
1 change: 0 additions & 1 deletion packages/react/src/components/ComboButton/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,6 @@ const ComboButton = React.forwardRef<HTMLDivElement, ComboButtonProps>(
ref={refs.setFloating}
id={id}
label={t('carbon.combo-button.additional-actions')}
mode="basic"
size={size}
open={open}
onClose={handleClose}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ interface ContextMenuProps {
x: number;
y: number;
onClose: () => void;
mode: string;
}

/**
Expand Down Expand Up @@ -62,7 +61,6 @@ function useContextMenu(trigger: TriggerType = document): ContextMenuProps {
x: position[0],
y: position[1],
onClose,
mode: 'full',
};
}

Expand Down
44 changes: 0 additions & 44 deletions packages/react/src/components/Menu/Menu-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,21 +83,6 @@ describe('Menu', () => {
expect(document.querySelector('.custom-class')).toBeInTheDocument();
document.body.removeChild(el);
});

it('warns about nested menus in basic mode', async () => {
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {});

render(
<Menu open mode="basic">
<MenuItem label="Submenu">
<MenuItem label="Item" />
</MenuItem>
</Menu>
);
await waitForPosition();
expect(spy).toHaveBeenCalled();
spy.mockRestore();
});
});

describe('Submenu behavior', () => {
Expand Down Expand Up @@ -224,35 +209,6 @@ describe('MenuItem', () => {

expect(screen.getByText('item')).toBeInTheDocument();
});

it('warns about MenuItemSelectable in basic mode', () => {
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {});

render(
<Menu open mode="basic">
<MenuItemSelectable label="Option" />
</Menu>
);

expect(spy).toHaveBeenCalled();
spy.mockRestore();
});

it('warns about MenuItemRadioGroup in basic mode', () => {
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {});

render(
<Menu open mode="basic">
<MenuItemRadioGroup
label="Options"
items={['Option 1', 'Option 2']}
/>
</Menu>
);

expect(spy).toHaveBeenCalled();
spy.mockRestore();
});
});

it('should call MenuItemRadioGroup onChange once', async () => {
Expand Down
44 changes: 39 additions & 5 deletions packages/react/src/components/Menu/Menu.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@
import React from 'react';
import { action } from '@storybook/addon-actions';

import {
Copy,
Cut,
FolderShared,
Paste,
TextBold,
TextItalic,
TrashCan,
} from '@carbon/icons-react';

import {
Menu,
MenuItem,
Expand Down Expand Up @@ -49,7 +59,7 @@ export const Default = (args) => {

return (
<Menu {...args} target={target} x={document?.dir === 'rtl' ? 250 : 0}>
<MenuItem label="Share with">
<MenuItem label="Share with" renderIcon={FolderShared}>
<MenuItemRadioGroup
label="Share with"
items={['None', 'Product team', 'Organization', 'Company']}
Expand All @@ -58,17 +68,40 @@ export const Default = (args) => {
/>
</MenuItem>
<MenuItemDivider />
<MenuItem label="Cut" shortcut="⌘X" onClick={itemOnClick} />
<MenuItem label="Copy" shortcut="⌘C" onClick={itemOnClick} />
<MenuItem label="Paste" shortcut="⌘V" disabled onClick={itemOnClick} />
<MenuItem
label="Cut"
shortcut="⌘X"
onClick={itemOnClick}
renderIcon={Cut}
/>
<MenuItem
label="Copy"
shortcut="⌘C"
onClick={itemOnClick}
renderIcon={Copy}
/>
<MenuItem
label="Paste"
shortcut="⌘V"
disabled
onClick={itemOnClick}
renderIcon={Paste}
/>
<MenuItemDivider />
<MenuItemGroup label="Font style">
<MenuItemSelectable
label="Bold"
shortcut="⌘B"
defaultSelected
onChange={selectableOnChange}
renderIcon={TextBold}
/>
<MenuItemSelectable
label="Italic"
shortcut="⌘I"
onChange={selectableOnChange}
renderIcon={TextItalic}
/>
<MenuItemSelectable label="Italic" onChange={selectableOnChange} />
</MenuItemGroup>
<MenuItemDivider />
<MenuItemRadioGroup
Expand All @@ -83,6 +116,7 @@ export const Default = (args) => {
shortcut="⌫"
kind="danger"
onClick={itemOnClick}
renderIcon={TrashCan}
/>
</Menu>
);
Expand Down
21 changes: 10 additions & 11 deletions packages/react/src/components/Menu/Menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { createPortal } from 'react-dom';
import { keys, match } from '../../internal/keyboard';
import { useMergedRefs } from '../../internal/useMergedRefs';
import { usePrefix } from '../../internal/usePrefix';
import { warning } from '../../internal/warning.js';
import deprecate from '../../prop-types/deprecate';

import { MenuContext, menuReducer } from './MenuContext';
import { useLayoutDirection } from '../LayoutDirection';
Expand Down Expand Up @@ -57,6 +57,7 @@ interface MenuProps extends React.HTMLAttributes<HTMLUListElement> {
menuAlignment?: string;

/**
* @deprecated Menus now always support both icons as well as selectable items and nesting.
* The mode of this menu. Defaults to full.
* `full` supports nesting and selectable menu items, but no icons.
* `basic` supports icons but no nesting or selectable menu items.
Expand Down Expand Up @@ -110,7 +111,7 @@ const Menu = forwardRef<HTMLUListElement, MenuProps>(function Menu(
containerRef,
label,
menuAlignment,
mode = 'full',
mode,
onClose,
onOpen,
open,
Expand All @@ -132,19 +133,11 @@ const Menu = forwardRef<HTMLUListElement, MenuProps>(function Menu(

const isRoot = context.state.isRoot;

if (context.state.mode === 'basic' && !isRoot) {
warning(
false,
'Nested menus are not supported when the menu is in "basic" mode.'
);
}

const menuSize = isRoot ? size : context.state.size;

const [childState, childDispatch] = useReducer(menuReducer, {
...context.state,
isRoot: false,
mode,
size,
requestCloseRoot: isRoot ? handleClose : context.state.requestCloseRoot,
});
Expand Down Expand Up @@ -424,6 +417,8 @@ const Menu = forwardRef<HTMLUListElement, MenuProps>(function Menu(
[`${prefix}--menu--shown`]:
(open && !legacyAutoalign) || (position[0] >= 0 && position[1] >= 0),
[`${prefix}--menu--with-icons`]: childContext.state.hasIcons,
[`${prefix}--menu--with-selectable-items`]:
childContext.state.hasSelectableItems,
[`${prefix}--autoalign`]: !legacyAutoalign,
}
);
Expand Down Expand Up @@ -474,13 +469,17 @@ Menu.propTypes = {
menuAlignment: PropTypes.string,

/**
* **Deprecated**: Menus now always support both icons as well as selectable items and nesting.
* The mode of this menu. Defaults to full.
* `full` supports nesting and selectable menu items, but no icons.
* `basic` supports icons but no nesting or selectable menu items.
*
* **This prop is not intended for use and will be set by the respective implementation (like useContextMenu, MenuButton, and ComboButton).**
*/
mode: PropTypes.oneOf(['full', 'basic']),
mode: deprecate(
PropTypes.oneOf(['full', 'basic']),
'Menus now always support both icons as well as selectable items and nesting.'
),

/**
* Provide an optional function to be called when the Menu should be closed.
Expand Down
13 changes: 9 additions & 4 deletions packages/react/src/components/Menu/MenuContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,23 @@
import { createContext, KeyboardEvent, RefObject } from 'react';

type ActionType = {
type: 'enableIcons' | 'registerItem';
type: 'enableIcons' | 'enableSelectableItems' | 'registerItem';
payload: any;
};

type StateType = {
isRoot: boolean;
mode: 'full' | 'basic';
hasIcons: boolean;
hasSelectableItems: boolean;
size: 'xs' | 'sm' | 'md' | 'lg' | null;
items: any[];
requestCloseRoot: (e: Pick<KeyboardEvent<HTMLUListElement>, 'type'>) => void;
};

const menuDefaultState: StateType = {
isRoot: true,
mode: 'full',
hasIcons: false,
hasSelectableItems: false,
size: null,
items: [],
requestCloseRoot: () => {},
Expand All @@ -37,6 +37,11 @@ function menuReducer(state: StateType, action: ActionType) {
...state,
hasIcons: true,
};
case 'enableSelectableItems':
return {
...state,
hasSelectableItems: true,
};
case 'registerItem':
return {
...state,
Expand All @@ -48,7 +53,7 @@ function menuReducer(state: StateType, action: ActionType) {
}

type DispatchFuncProps = {
type: 'registerItem' | 'enableIcons';
type: ActionType['type'];
payload: {
ref: RefObject<HTMLLIElement>;
disabled: boolean;
Expand Down
Loading
Loading