view_components icon indicating copy to clipboard operation
view_components copied to clipboard

Allow nested sub-menus in the ActionMenu component

Open camertron opened this issue 7 months ago • 1 comments

What are you trying to accomplish?

This PR adds multi-level support to the ActionMenu component. It enables menus to be nested inside other menus just as can be done for the corresponding React component.

Since there was no reason to build it, and because it's not clear how it should work, ActionMenus with sub-menus do not support single-select mode.

Screenshots

Alt: A screen recording initially showing only a button labeled "Edit." When the button is clicked, an ActionMenu appears containing three items: "Cut," "Copy," and "Paste special." The last item's label is followed by a right-facing chevron icon. This last item is clicked, which causes a sub-menu to appear to the right. This sub-menu contains four items, the last of which also features a right-facing chevron icon. When the last item is clicked, a third sub-menu appears containing 3 additional items. The last item is clicked and all three menus disappear.

https://github.com/user-attachments/assets/5eff167c-5720-4404-99e7-1ef30d72ee97

Integration

Multi-level support has been designed to maintain feature parity with the current ActionMenu component so as to be entirely backwards-compatible, at least from an API perspective. The component should generate nearly identical markup with the minor exception of the newly added sub-menu item, which includes a nested <ul> and <anchored-position>.

However, care should be taken when integrating these changes, since they do represent a significant re-architecting of the component's inner workings.

Risk Assessment

  • [ ] Low risk the change is small, highly observable, and easily rolled back.
  • [x] Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • [ ] High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

This feature was built by introducing a new menu item class called SubMenuItem. This class represents the menu item element and renders a SubMenu component that essentially contains another ActionMenu wrapped in its own <anchored-position>. To share code between the primary menu and sub-menus, the original code from the ActionMenu class was moved into a base class named Menu, from which both PrimaryMenu and SubMenu now inherit.

Accessibility

  • No new axe scan violation - This change does not introduce any new axe scan violations.

Merge checklist

  • [x] Added/updated tests
  • [x] Added/updated documentation
  • [x] Added/updated previews (Lookbook)
  • [x] Tested in Chrome
  • [x] Tested in Firefox
  • [x] Tested in Safari
  • [x] Tested in Edge

camertron avatar Jun 23 '25 05:06 camertron

🦋 Changeset detected

Latest commit: 8210923108d983b690bf2e58c08e513b743682bf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Jun 23 '25 05:06 changeset-bot[bot]

Thanks for the contribution, @camertron! @hectahertz this one has been open for two weeks, can you please try to prioritize a review this week? Thanks!

lesliecdubs avatar Jul 10 '25 03:07 lesliecdubs

Thanks for your contribution, @camertron! Sorry for the delay in getting back to you. We've been having some discussions about finding the right balance between the long-term maintenance commitment and the advantages of having our component library more in sync with its React counterpart.

Could you please take a look at the CI and clear up those lint errors? Once that's done, I can get this merged into the next release.

Moving forward, if you're considering bigger changes or additions to the library, let's connect first. That way, we can develop a plan that suits us both.

hectahertz avatar Jul 31 '25 17:07 hectahertz

Hi @hectahertz

thanks for taking the time to review this 🙇 We originally did not expect such a big code change for this feature, which is why we just went ahead. We'd be happy to align up in front before we start on bigger changes next time 👍

Regarding the CI, I only see the changeset and the semver label checks failing (which always fail due to the fork setup). I don't know if I am lacking permission or whether the CI has re-run in the meantime, but I don't see any lint errors. If you could provide me a link or a screenshot, I'd be happy to fix the issues in case they still exist.

HDinger avatar Aug 04 '25 06:08 HDinger