Skip to content

Commit 2637e04

Browse files
LukasTyclaude
andcommitted
Accept every node kind as the root trigger
`trigger` is typed as a node, but only some nodes worked. The check used `React.isValidElement`, which is true for a fragment. A fragment then took the render path, and it cannot take props or a ref, so no trigger element rendered at all and React logged an error. The root now treats a fragment as content, the same as text or several nodes. A single element still becomes the trigger itself. Corrects the prop documentation and the RFC. Both said the root "takes an element", which hid this gap. They now say the prop takes a node, and they name what each node kind does. A test covers text, a fragment, and several nodes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 94e2d9d commit 2637e04

3 files changed

Lines changed: 40 additions & 14 deletions

File tree

docs/pages/experiments/menu2-rfc.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -216,9 +216,9 @@ The trigger part and the popup part still exist, but they are internal. We expor
216216

217217
Three results come from this work:
218218

219-
- **The root `trigger` takes an element.** Base UI's `render` merges the behavior into that element. Thus the caller keeps their own component. Other content renders inside the default trigger.
220-
- **The submenu `trigger` takes content, not an element.** This is the one place where the two levels cannot match. A submenu trigger is already a menu item. A `<Menu2Item>` in the `trigger` puts an item inside an item, and the submenu does not open.
221-
- **A `Tooltip` around a submenu trigger is now more difficult.** The root `trigger` takes an element, so `trigger={<Tooltip><Button /></Tooltip>}` still works. The submenu `trigger` takes content, so the wrapper must move into the root slot of the trigger, and that needs a `forwardRef` component. The test suite and the recipes page both show this.
219+
- **Both `trigger` props take a node.** For the root, a single element becomes the trigger itself, and Base UI's `render` merges the behavior into it, so the caller keeps their own component. Any other node, such as text, a fragment, or several nodes, renders inside the default trigger. A fragment is a valid element, but it cannot take props or a ref, so the root treats it as content.
220+
- **The submenu `trigger` is always content.** This is the one place where the two levels cannot match. A submenu trigger is already a menu item, so an element in the `trigger` does not replace it. A `<Menu2Item>` there puts an item inside an item, and the submenu does not open. Use `slots.trigger` to change the component.
221+
- **A `Tooltip` around a submenu trigger is now more difficult.** The root `trigger` accepts an element, so `trigger={<Tooltip><Button /></Tooltip>}` still works. The submenu `trigger` is content, so the wrapper must move into the root slot of the trigger, and that needs a `forwardRef` component. The test suite and the recipes page both show this.
222222

223223
For the classic controlled pattern, omit `trigger` and control the menu with `open` and `anchor`. The context-menu recipe uses this pattern.
224224

packages/mui-material/src/Unstable_Menu2/Menu2.tsx

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,13 @@ export interface Menu2Props
3737
*/
3838
children?: React.ReactNode;
3939
/**
40-
* The element that opens the menu.
40+
* The content that opens the menu.
4141
*
42-
* An element is rendered as-is with the trigger behavior merged into it, so
43-
* it keeps whatever component you passed. Anything else renders inside the
44-
* default trigger. Omit it and drive the menu with `open` and `anchor`
45-
* instead, which is the classic controlled pattern.
42+
* A single element becomes the trigger itself: the trigger behavior merges
43+
* into it, so it keeps whatever component you passed. Any other node, such as
44+
* text, a fragment, or several nodes, renders inside the default trigger.
45+
* Omit it and drive the menu with `open` and `anchor` instead, which is the
46+
* classic controlled pattern.
4647
*/
4748
trigger?: React.ReactNode;
4849
/**
@@ -105,9 +106,13 @@ const Menu2 = React.forwardRef(function Menu2(
105106
const { trigger: triggerSlotProps, ...popupSlotProps } = slotProps ?? {};
106107
const resolvedTriggerProps = resolveComponentProps(triggerSlotProps, themedProps);
107108

109+
// A fragment is a valid element but cannot take the trigger's props or ref,
110+
// so it counts as content and goes inside the default trigger.
111+
const triggerIsElement = React.isValidElement(trigger) && trigger.type !== React.Fragment;
112+
108113
let triggerNode: React.ReactNode = null;
109114
if (trigger != null) {
110-
triggerNode = React.isValidElement(trigger) ? (
115+
triggerNode = triggerIsElement ? (
111116
// Base UI's `render` merges the trigger behavior into the element, so the
112117
// caller keeps whatever component they passed.
113118
<BaseMenu.Trigger
@@ -199,12 +204,13 @@ Menu2.propTypes /* remove-proptypes */ = {
199204
trigger: PropTypes.elementType,
200205
}),
201206
/**
202-
* The element that opens the menu.
207+
* The content that opens the menu.
203208
*
204-
* An element is rendered as-is with the trigger behavior merged into it, so
205-
* it keeps whatever component you passed. Anything else renders inside the
206-
* default trigger. Omit it and drive the menu with `open` and `anchor`
207-
* instead, which is the classic controlled pattern.
209+
* A single element becomes the trigger itself: the trigger behavior merges
210+
* into it, so it keeps whatever component you passed. Any other node, such as
211+
* text, a fragment, or several nodes, renders inside the default trigger.
212+
* Omit it and drive the menu with `open` and `anchor` instead, which is the
213+
* classic controlled pattern.
208214
*/
209215
trigger: PropTypes.node,
210216
} as any;

packages/mui-material/src/Unstable_Menu2Submenu/Menu2Collapsed.test.tsx

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,26 @@ describe('<Menu2 /> collapsed API', () => {
174174
expect(Math.round(submenu.top)).to.equal(Math.round(triggerRect.top) - 8);
175175
});
176176

177+
// `trigger` is typed as a node, so every node kind has to work. A fragment is
178+
// a valid element but cannot take props or a ref, so it must count as content.
179+
it.each([
180+
['text', 'Options'],
181+
['a fragment', <React.Fragment key="f">Options</React.Fragment>],
182+
['several nodes', ['Options', <span key="s" />]],
183+
])('accepts %s as the trigger', async (_name, triggerValue) => {
184+
const { user } = render(
185+
<Menu2 trigger={triggerValue as React.ReactNode}>
186+
<Menu2Item>Profile</Menu2Item>
187+
</Menu2>,
188+
);
189+
190+
const trigger = screen.getByRole('button');
191+
expect(trigger).to.have.class(menu2TriggerClasses.root);
192+
193+
await user.click(trigger);
194+
expect(await screen.findByRole('menu')).not.to.equal(null);
195+
});
196+
177197
it('falls back to the default trigger for a non-element', async () => {
178198
const { user } = render(
179199
<Menu2 trigger="Options">

0 commit comments

Comments
 (0)