fix(MegaMenu): support multiple dropdowns in a single mega menu - #1685
Open
jorj-pineda wants to merge 7 commits into
Open
fix(MegaMenu): support multiple dropdowns in a single mega menu#1685jorj-pineda wants to merge 7 commits into
jorj-pineda wants to merge 7 commits into
Conversation
Add a fixture rendering two MegaMenuDropdownToggle/MegaMenuDropdown pairs inside a single MegaMenu, covering the scenario reported in themesberg#1610. Both new tests fail against the current implementation: - Every toggle resolves its dropdown via querySelector('[role=menu]') scoped to the whole <nav>, so all toggles target the first dropdown. aria-controls on the second toggle points at the first dropdown's id. - Clicking the second toggle hides/shows the first dropdown instead of its own. Refs themesberg#1610
MegaMenuDropdownToggle resolved its dropdown with querySelector('[role=menu]')
scoped to the enclosing <nav>, which always returns the first match. Every toggle
in a mega menu therefore controlled the first dropdown, and MegaMenuDropdown had
the mirror-image bug, labelling every panel with the first toggle's id.
Pair them by declaration order instead: the n-th toggle controls the n-th
dropdown. Pairing keys off dedicated data attributes rather than the ARIA roles,
because <MegaMenuDropdown toggle={..}> renders a Dropdown that also carries
aria-haspopup=menu and role=menu — and mounts its panel only while open,
which would shift the indices as sibling dropdowns opened and closed.
Also extract the panel branch of MegaMenuDropdown into its own component. Its
useId/useRef/useEffect calls sat after the early return for the variant,
so they were called conditionally, breaking the rules of hooks.
Fixes themesberg#1610
Multiple mega menu dropdowns could be expanded at the same time, since each toggle only ever touched its own dropdown. Track the currently open toggle in a new MegaMenuContext so a toggle can collapse itself when a sibling opens. Coordinating through context rather than reaching into the sibling's DOM keeps each toggle's aria-expanded owned by the component that renders it, so the attribute cannot drift from what is visible. The context is only read on interaction, so panels declared without a class still render open on mount as before. useMegaMenuContext intentionally returns undefined instead of throwing, because MegaMenuDropdownToggle also works inside a plain Navbar. Adjusts the multiple-dropdown test that asserted the previous non-exclusive behavior on re-open. Refs themesberg#1610
Every other context in the library is part of the public API (NavbarContext, DropdownContext, SidebarContext, AccordionPanelContext), so export MegaMenuContext, useMegaMenuContext and MegaMenuContextValue for consistency. The pairing helpers stay internal, matching how Clipboard and Pagination keep their helpers.ts unexported. Refs themesberg#1610
Document declaring more than one dropdown in a single mega menu: pairing goes by declaration order, toggles and dropdowns do not have to be siblings, and a "hidden" class makes a panel start closed. Refs themesberg#1610
Patch bump, matching how this project versions additive changes: minor is reserved for breaking changes in the changelog history. Refs themesberg#1610
🦋 Changeset detectedLatest commit: 6e5366b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@jorj-pineda is attempting to deploy a commit to the Bergside Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughMegaMenu now supports multiple declaration-ordered dropdown pairs. Shared context tracks the active toggle and closes other panels. Tests, Storybook, web examples, documentation, and a patch changeset cover the new behavior. ChangesMultiple MegaMenu dropdowns
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MegaMenuDropdownToggle
participant MegaMenuDropdown
participant MegaMenuContext
User->>MegaMenuDropdownToggle: click toggle
MegaMenuDropdownToggle->>MegaMenuDropdown: toggle paired panel
MegaMenuDropdownToggle->>MegaMenuContext: set active toggle ID
MegaMenuContext-->>MegaMenuDropdownToggle: notify other toggles
MegaMenuDropdownToggle->>MegaMenuDropdown: hide previously open panel
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/MegaMenu/MegaMenuDropdownToggle.tsx`:
- Around line 40-51: Update MegaMenuDropdownToggle’s onClick handler to stop
event propagation before toggling the paired dropdown, preventing the parent
NavbarLink from closing NavbarCollapse. Add an integration test covering
NavbarToggle, NavbarCollapse, and a nested MegaMenuDropdownToggle, verifying the
mobile navbar remains open when the dropdown is toggled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 198cbcd9-1662-4b7c-aafa-bd94d9a40b14
📒 Files selected for processing (12)
.changeset/olive-donkeys-shave.mdapps/storybook/src/MegaMenu.stories.tsxapps/web/content/docs/components/mega-menu.mdxapps/web/examples/megaMenu/index.tsapps/web/examples/megaMenu/megaMenu.multipleDropdowns.tsxpackages/ui/src/components/MegaMenu/MegaMenu.test.tsxpackages/ui/src/components/MegaMenu/MegaMenu.tsxpackages/ui/src/components/MegaMenu/MegaMenuContext.tsxpackages/ui/src/components/MegaMenu/MegaMenuDropdown.tsxpackages/ui/src/components/MegaMenu/MegaMenuDropdownToggle.tsxpackages/ui/src/components/MegaMenu/helpers.tspackages/ui/src/components/MegaMenu/index.ts
Children of MegaMenu are flex items of a "flex flex-wrap justify-between" row, so a dropdown without a width class renders beside the navbar links instead of below them, and the links shift whenever it is hidden. Add w-full so each dropdown wraps onto its own row, and document it. Refs themesberg#1610
2 tasks
Author
DemoRecorded against the Storybook story added in this PR: 2026-08-11.21-02-17.mp4 |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the functional half of #1610: a
<MegaMenu>containing more than one dropdown only ever operated the first one.<MegaMenuDropdownToggle>resolved its dropdown withquerySelector('[role="menu"]')scoped to the enclosing<nav>.querySelectorreturns the first match, so every toggle in the navbar controlled the first dropdown.<MegaMenuDropdown>had the mirror-image bug, pointing every panel'saria-labelledbyat the first toggle's id.Changes
<NavbarCollapse>while the dropdown sits outside it.MegaMenuContextso each toggle'saria-expandedstays owned by the component that renders it, rather than being written into a sibling's DOM where it could drift from what is actually visible.<MegaMenuDropdown>calleduseId,useRefanduseEffectafter its early return for thetogglevariant. The panel branch is now its own component, so the hooks are unconditional.Implementation note
Pairing keys off two internal
data-*attributes rather than the ARIA roles.<MegaMenuDropdown toggle={..}>renders a<Dropdown>whose trigger also carriesaria-haspopup="menu"and whose floating panel also carriesrole="menu"— and that panel is only mounted while open. Indexing on the roles would therefore have re-shuffled the pairing every time a sibling dropdown opened or closed.Breaking API changes
None. No props were added, removed or renamed, and the theme structure is unchanged. The two
data-*attributes are inert markers and are not exported.MegaMenuContext,useMegaMenuContextandMegaMenuContextValueare newly exported, matching howNavbarContext,DropdownContext,SidebarContextandAccordionPanelContextare already part of the public API.One behavior change worth calling out: with several dropdowns declared, opening one now closes the others. Mount behavior is untouched — a panel declared without a
hiddenclass still renders open on first paint, exactly as before.Deliberately not included
#1610 also reports that a
<MegaMenuDropdown toggle={..}>cannot be reliably centered. That one is not a local bug:classNameis forwarded to both the trigger and the floating panel, and the panel carries inline positioning styles from Floating UI that a utility class cannot override. Fixing it means an API decision — a separatefloatingClassName, a positioning escape hatch, or something else — so I have left it out rather than guess. Happy to follow up in a separate PR if you can indicate the direction you would prefer.Note also that #1666 already proposes a one-line
theme.tschange for the related margin issue. This PR does not touchtheme.ts, so the two should not conflict.Testing
packages/ui/src/components/MegaMenu/MegaMenu.test.tsxgains coverage for two toggle/dropdown pairs:aria-controlsand each panel'saria-labelledbyresolve to their own partnerBoth new pairing tests fail on
mainand pass with this change. The existing single-dropdown tests are unaffected.The multiple-dropdown isolation test drops two assertions that described the previous non-exclusive behavior on re-open, since that is what the exclusive-open change replaces.
Refs #1610
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests