Skip to content

feat: implement Menu component - #738

Open
patricklx wants to merge 3 commits into
mainfrom
fix-issue-694
Open

feat: implement Menu component#738
patricklx wants to merge 3 commits into
mainfrom
fix-issue-694

Conversation

@patricklx

Copy link
Copy Markdown
Collaborator

Summary

  • Implements Menu for parity with Carbon React: a context-menu-style component supporting nested submenus and keyboard navigation.
  • Adds MenuItem, MenuItemDivider, MenuItemGroup, MenuItemRadioGroup, and MenuItemSelectable subcomponents, including controlled/uncontrolled selection state.
  • Adds a docs page with live-preview examples and API reference.
  • Adds integration tests covering rendering, open/close, submenu navigation, and selection state.

Closes #694

Test plan

  • pnpm build succeeds
  • pnpm lint passes
  • pnpm test — full suite passes, including new Menu tests

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

📖 Docs Preview

🔗 Preview URL: https://ibm.github.io/carbon-components-ember/pr-previews/pr-738/

Built from commit 4cd9456

github-actions Bot pushed a commit that referenced this pull request Aug 5, 2026
github-actions Bot pushed a commit that referenced this pull request Aug 5, 2026
github-actions Bot pushed a commit that referenced this pull request Aug 5, 2026
@patricklx

Copy link
Copy Markdown
Collaborator Author

the docs preview for menu hangs.
debugged a bit: it looks like looping around ""hasSelectableItems" and "hasIcons"

@patricklx patricklx added review and removed review labels Aug 5, 2026
github-actions Bot pushed a commit that referenced this pull request Aug 5, 2026
@patricklx

Copy link
Copy Markdown
Collaborator Author

in docs preview the menu shows up in left corner. can it be contained to the docs root (shadow dom)?
also, regarding the mutation observer. is there a better, more ember-ish way?

@patricklx

Copy link
Copy Markdown
Collaborator Author

Thanks — both of those were the same root cause plus a docs-preview issue. Pushed cbfdc737.

The hang (hasIcons/hasSelectableItems loop)

You were right about where it looped. The MutationObserver watched its own <ul> with attributes: true, so every class rewrite Ember did for --with-icons/--with-selectable-items fed straight back into the observer callback. That callback then set tracked state the same render had already consumed via classes — a backtracking-rerender assertion in dev, and an endless revalidation loop in a production build, which is what the docs preview hit. The equality guard I added in 4ba28234 wasn't enough because the observer also fires on the style writes from positionMenu and on the async icon SVGs landing.

The more ember-ish way

Dropped the observer and the DOM scanning entirely. Items now register with the menu they belong to, which is both what Carbon's React version effectively does (items feed the context the classes are derived from) and what ProgressIndicator/Tabs already do in this repo:

  • Menu registers itself under its own <ul> in a module-level WeakMap from a modifier.
  • MenuItem looks its owner up once on insert (element.parentElement.closest('.cds--menu')) and calls registerItem/unregisterItem, with the lookup deferred via runTask because Glimmer installs a child's modifiers before its parent's.
  • Menu.hasIcons/hasSelectableItems are now plain getters over the registered items (item.hasIcon = @renderIcon was passed, item.isSelectable = role is menuitemcheckbox/menuitemradio).

Side benefits: the classes no longer wait for an icon's async SVG to land, they stay reactive when items are added/removed at runtime, and submenus track their own items rather than the root menu's. Two new tests cover exactly that.

Menu showing up in the left corner of the docs preview

That's the document.body default portal target: the live preview renders in a shadow root, so a menu portaled to document.body lands outside it, where the preview's Carbon styles don't reach it — hence an unstyled block at 0,0.

The Menu already takes @target (parity with React's target prop), so the docs examples now pass a local element, the same way portal.md does. The demo box also carries a transform, which makes it the containing block for the position: fixed menu, so @x/@y are relative to the box and the menu stays inside the preview and scrolls with it. Prose in the docs explains both, including what you'd pass in a real app that renders into document.body.

Testing

  • pnpm build (addon glint + rollup) clean.
  • Full test-app suite: all 12 Menu tests pass, including the 2 new ones. The 8 failures that remain are pre-existing style-snapshot drift in Breadcrumbs/IconIndicator/ShapeIndicator/Link — I verified they fail identically with this branch's changes reverted (443/451 before, 445/453 after — same 8).

@patricklx patricklx removed the review label Aug 5, 2026
github-actions Bot pushed a commit that referenced this pull request Aug 5, 2026
github-actions Bot pushed a commit that referenced this pull request Aug 5, 2026
@patricklx

Copy link
Copy Markdown
Collaborator Author

menu is now in shadow dom, but sub menu (opened to the right) is not correctly positioned

Implements Menu for parity with Carbon React: a context-menu-style
component with MenuItem, MenuItemDivider, MenuItemGroup,
MenuItemRadioGroup, and MenuItemSelectable, supporting nested submenus,
keyboard navigation, and controlled/uncontrolled selection state.

Closes #694

Signed-off-by: Patrick Pircher <patrick.pircher@ibm.com>
…r assertion, and fix menu-test to assert against the portaled DOM

scanFeatures unconditionally reassigned the @Tracked hasIcons/
hasSelectableItems properties on every scan, which both retriggered its own
MutationObserver in a loop and rewrote the class attribute during the same
render computation that had already read it. Only assign when the computed
value actually changes, and defer the initial scan to the next frame like
positionMenu already does.

menu-test.gts rendered the root Menu with its default @target
(document.body) and asserted with the default qunit-dom context, which only
searches the rendering-test container - not document.body, where the
in-element portal actually places content. Every test now points @target at
a dedicated container appended to the body and scopes assertions to it,
mirroring the workaround already used in portal-test.gts.

Signed-off-by: Patrick Pircher <patrick.pircher@ibm.com>
The observer watched its own `<ul>`'s attributes, so every `class` rewrite
Ember performed for the `--with-icons`/`--with-selectable-items` modifier
classes fed straight back into the observer callback. That callback set
tracked state the same render had already consumed, which is a
backtracking-rerender assertion in dev builds and an endless revalidation
loop in production - the docs preview hanging.

Items now register with the menu they belong to, the way Carbon's React
items feed the context the classes are derived from, and this repo's own
ProgressIndicator/Tabs do. `Menu` derives both classes from the registered
items, so they stay reactive to items being added or removed and no longer
depend on an icon's async SVG landing in the DOM.

Also contains the Menu docs examples inside the preview: they pass a local
element as `@target` so the menu renders in the preview's shadow root
instead of escaping into `document.body`, where the preview's styles don't
reach it and it showed up unstyled in the page's top left corner.

Signed-off-by: Patrick Pircher <patrick.pircher@ibm.com>
github-actions Bot pushed a commit that referenced this pull request Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Parity Check] Investigate Menu component

1 participant