fix: separate navigation and dropdown toggle in mega menu #4831
Conversation
- Split NavItem click handlers into text navigation and arrow toggle - Text/link now navigates to the page - Arrow icon toggles dropdown menu - Improves UX by allowing users to go directly to pages Fixes asyncapi#4807
debasish5452v
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
📝 WalkthroughWalkthroughNavItem gained an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2024-10-11T11:32:30.226ZApplied to files:
📚 Learning: 2024-10-11T07:38:35.745ZApplied to files:
📚 Learning: 2024-10-11T10:46:58.882ZApplied to files:
📚 Learning: 2025-12-29T14:21:28.216ZApplied to files:
🪛 GitHub Actions: PR testing - if Node projectcomponents/navigation/NavItem.tsx[error] 72-72: ESLint: This line has a length of 211. Maximum allowed is 120. max-len 🔇 Additional comments (3)
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 |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4831--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/navigation/NavBar.tsxcomponents/navigation/NavItem.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/navigation/NavItem.tsx
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
components/navigation/NavItem.tsx
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/navigation/NavItem.tsx
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/navigation/NavItem.tsxcomponents/navigation/NavBar.tsx
🪛 GitHub Actions: PR testing - if Node project
components/navigation/NavItem.tsx
[error] 69-69: This line has a length of 211. Maximum allowed is 120. max-len
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Lighthouse CI
🔇 Additional comments (1)
components/navigation/NavItem.tsx (1)
13-13: LGTM: Clean addition of onDropdownClick prop.The new prop is properly declared in the interface, documented in JSDoc, and destructured in the function signature.
Also applies to: 25-25, 35-35
| text='Docs' | ||
| href='/docs' | ||
| onClick={() => showOnClickMenu('learning')} | ||
| onDropdownClick={() => showOnClickMenu('learning')} |
There was a problem hiding this comment.
Arrow click only works on mobile—contradicts PR objectives.
The onDropdownClick handlers call showOnClickMenu, which returns early on non-mobile devices (line 133: if (!isMobile) return;). This means clicking the arrow icon will do nothing on desktop/tablet devices.
PR objectives state:
- "Clicking the arrow icon expands/collapses the corresponding mega menu"
- "Provide non-hover access to mega menus (arrow click should open them)"
The arrow button should toggle the dropdown on all devices, not just mobile. The current implementation only provides hover access on desktop, which fails to address the core issue reported in #4807.
🔎 Proposed fix to enable arrow clicks on all devices
Update showOnClickMenu to toggle the dropdown regardless of device:
function showOnClickMenu(menu: 'learning' | 'tooling' | 'community' | null) {
- if (!isMobile) return;
if (open === menu) {
setOpen(null);
} else {
setOpen(menu);
}
}Alternatively, create a separate handler specifically for dropdown button clicks:
+function toggleDropdownMenu(menu: 'learning' | 'tooling' | 'community' | null) {
+ if (open === menu) {
+ setOpen(null);
+ } else {
+ setOpen(menu);
+ }
+}
+
function showOnClickMenu(menu: 'learning' | 'tooling' | 'community' | null) {
if (!isMobile) return;
if (open === menu) {
setOpen(null);
} else {
setOpen(menu);
}
}Then use toggleDropdownMenu for onDropdownClick:
<NavItem
text='Docs'
href='/docs'
- onDropdownClick={() => showOnClickMenu('learning')}
+ onDropdownClick={() => toggleDropdownMenu('learning')}
onMouseEnter={() => showMenu('learning')}
hasDropdown
/>Apply the same change to Tools and Community nav items.
Also applies to: 197-197, 208-208
🤖 Prompt for AI Agents
In components/navigation/NavBar.tsx around lines 186 (also applies to 197 and
208), the onDropdownClick handlers call showOnClickMenu which returns early on
non-mobile devices, so arrow clicks do nothing on desktop/tablet; update the
logic so arrow clicks toggle the dropdown on all devices by either removing the
isMobile early return in showOnClickMenu and implementing toggle behavior (open
if closed, close if open) or add a separate handler (e.g., toggleDropdownMenu)
that toggles the corresponding menu unconditionally and replace the
onDropdownClick bindings for Learning, Tools and Community to call that new
handler; ensure state updates and any side-effects (e.g., closing others) remain
consistent.
| if (href) { | ||
| return ( | ||
| <Link | ||
| href={href} | ||
| {...attrs} | ||
| className={`${attrs.className} ${router.pathname.startsWith(href) ? 'text-black' : 'text-gray-700'}`} | ||
| target={target} | ||
| data-testid='NavItem-Link' | ||
| > | ||
| <span>{text}</span> | ||
| {hasDropdown && <NavItemDropdown />} | ||
| </Link> | ||
| <div className='inline-flex items-center' onMouseEnter={onMouseEnter}> | ||
| <Link | ||
| href={href} | ||
| className={`group inline-flex items-center font-body text-base leading-6 font-semibold hover:text-gray-900 focus:outline-none focus:text-gray-900 tracking-heading transition ease-in-out duration-150 ${ | ||
| router.pathname.startsWith(href) ? 'text-black' : 'text-gray-700' | ||
| } ${className}`} | ||
| target={target} | ||
| data-testid='NavItem-Link' | ||
| > | ||
| <span>{text}</span> | ||
| </Link> | ||
| {hasDropdown && ( | ||
| <button | ||
| type='button' | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| if (onDropdownClick) onDropdownClick(); | ||
| }} | ||
| className='ml-2 text-gray-700 hover:text-gray-900 focus:outline-none' | ||
| aria-label='Toggle dropdown' | ||
| > | ||
| <NavItemDropdown /> | ||
| </button> | ||
| )} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's read the actual file to verify line 69 length
cat -n components/navigation/NavItem.tsx | sed -n '64,92p'Repository: asyncapi/website
Length of output: 1263
🏁 Script executed:
#!/bin/bash
# Check for linting configuration
fd -type f -name "\.eslintrc*" -o -name "eslint.config.*" | head -20Repository: asyncapi/website
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for NavItem usages with href, hasDropdown, and onClick combinations
rg -nP --type=tsx --type=ts 'NavItem\s+' -A 5 | grep -E '(href|hasDropdown|onClick)' -B 2 -A 2Repository: asyncapi/website
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Count exact characters on line 69
python3 << 'EOF'
line = " className={`group inline-flex items-center font-body text-base leading-6 font-semibold hover:text-gray-900 focus:outline-none focus:text-gray-900 tracking-heading transition ease-in-out duration-150 ${"
print(f"Line 69 character count: {len(line)}")
EOFRepository: asyncapi/website
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Find ESLint configuration files
find . -name ".eslintrc*" -o -name "eslint.config.*" 2>/dev/nullRepository: asyncapi/website
Length of output: 71
🏁 Script executed:
#!/bin/bash
# Search for NavItem component definition to understand props
rg -A 30 "interface NavItemProps|type NavItemProps|function NavItem|const NavItem" components/navigation/NavItem.tsxRepository: asyncapi/website
Length of output: 1945
🏁 Script executed:
#!/bin/bash
# Search for NavItem usages with proper glob patterns
rg "NavItem" --glob="*.tsx" --glob="*.ts" -B 2 -A 5 | grep -E "(href|hasDropdown|onClick)" -B 3 -A 3 | head -100Repository: asyncapi/website
Length of output: 6132
🏁 Script executed:
#!/bin/bash
# Check the ESLint configuration for line length rules
cat ./.eslintrcRepository: asyncapi/website
Length of output: 8172
🏁 Script executed:
#!/bin/bash
# Get exact character count of line 69
sed -n '69p' components/navigation/NavItem.tsx | wc -cRepository: asyncapi/website
Length of output: 63
🏁 Script executed:
#!/bin/bash
# Verify that line 69 is actually ignored by checking if it triggers linting
# Let's see if there are similar long className lines elsewhere that aren't flagged
rg -n "className=\{.*\}" components/navigation/NavItem.tsx -oRepository: asyncapi/website
Length of output: 137
The onClick prop is not invoked when both href and hasDropdown are present, breaking the component's API contract.
The separation of link navigation from dropdown toggle is architecturally sound. However, the new code path completely omits invoking the onClick prop when href && hasDropdown. This violates the documented NavItemProps interface, which declares onClick?: () => void as a supported callback. Although current usages in NavBar.tsx don't pass onClick for this scenario, the change breaks the component's public API contract and could cause issues for future consumers.
Additionally, line 69 is 212 characters and exceeds the 120-character limit, which may require formatting to comply with linting rules.
Ensure onClick is invoked when provided, or explicitly remove it from NavItemProps and update documentation to reflect that onClick is not supported for items with both href and hasDropdown.
🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[error] 69-69: This line has a length of 211. Maximum allowed is 120. max-len
🤖 Prompt for AI Agents
components/navigation/NavItem.tsx lines 64-92: the current branch for
href+hasDropdown never invokes the optional onClick prop and one line exceeds
120 chars; update the JSX so the Link and the dropdown toggle button both call
onClick when provided (e.g., add onClick handlers that guard with if (onClick)
onClick()), keep existing e.preventDefault() behavior for the button before
invoking onClick, and wrap/format the long className string into multiple
concatenated lines to satisfy the 120-character limit.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4831 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 798 798
Branches 146 146
=========================================
Hits 798 798 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Ensure onClick callback is invoked when both href and hasDropdown are present - Remove mobile-only restriction from arrow dropdown button - Fix line length to comply with 120-character limit - Addresses CodeRabbit review feedback
|
FYI: #4807 (comment) |
3 participants
Description
This PR fixes the mega menu navigation behavior where clicking the dropdown arrow would navigate to the page instead of toggling the dropdown menu.
Changes
NavItemcomponent to separate text/link navigation from arrow dropdown toggle/docs,/tools,/community)onDropdownClickprop toNavItemfor dropdown-specific click handlingNavBarto pass separate handlers for navigation and dropdown toggleMotivation
Previously, both the text and arrow icon would navigate to the page, making it difficult for users to access the dropdown menu content. This improvement gives users clear control over their navigation choice.
Testing
Fixes #4807
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.