Hambuger drop-down has no background coverage causing overlapping of homepage and drop down component #30
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe Navbar component is refactored from a complex multi-state desktop/mobile architecture to a simplified design featuring a mobile half-sheet menu. State management is consolidated, a backdrop and scroll-locking mechanism are introduced, and navigation logic is streamlined with minimal fixed navbar elements for desktop and a hamburger-triggered sheet for mobile. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant NavBar as Navbar Component
participant Sheet as Mobile Sheet Menu
participant Backdrop
participant Body as Body Element
participant Action as Navigation/Wallet Action
User->>NavBar: Tap hamburger menu button
NavBar->>NavBar: Set open = true
NavBar->>Backdrop: Render backdrop
NavBar->>Body: Lock scroll
Backdrop->>Backdrop: Display with overlay
rect rgba(100, 200, 255, 0.5)
Note over Sheet: Sheet is now visible
User->>Sheet: View nav links / wallet section
User->>Sheet: Interact with menu item or button
Sheet->>Action: Trigger navigation or wallet config
end
User->>Backdrop: Tap backdrop OR action completes
NavBar->>NavBar: Set open = false
NavBar->>Backdrop: Hide backdrop
NavBar->>Body: Unlock scroll
Backdrop->>Backdrop: Disappear
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/src/components/Navbar.tsx`:
- Around line 31-37: The useEffect that manages the scroll listener currently
runs only when isLanding is true, causing scrolled to remain stale on
non-landing pages; update the effect in Navbar (the useEffect that references
isLanding, setScrolled and window.addEventListener/ removeEventListener) to:
when isLanding is true attach the scroll listener as before, and when isLanding
is false immediately setScrolled(true) (and ensure no dangling listener is
attached), or alternatively always attach the scroll listener but default
scrolled to true for non-landing pages; make the change for both the first
effect (lines ~31-37) and the similar block around lines ~55-60 so non-landing
routes always render a solid background.
- Around line 22-28: The sheet's open state isn't reset on client-side
navigation; add an effect that closes the menu whenever the route changes by
using useEffect(() => setOpen(false), [pathname]) in the Navbar component
(references: open, setOpen, usePathname) so any navigation (back/forward or
link) will setOpen(false) and unlock body scroll on the new page.
🧹 Nitpick comments (3)
web/src/components/Navbar.tsx (3)
68-73: Addaria-labelto the hamburger and close buttons for accessibility.Screen readers can't determine the purpose of icon-only buttons without labels.
Proposed fix
<button onClick={() => setOpen(true)} - className="md:hidden text-white p-2" + className="md:hidden text-white p-2" + aria-label="Open menu" > <Menu size={26} /> </button>And for the close button (Line 98):
- <button onClick={() => setOpen(false)} className="text-white"> + <button onClick={() => setOpen(false)} className="text-white" aria-label="Close menu"> <X size={24} /> </button>
104-104: Hardcoded50vhin calc will drift from theSHEET_HEIGHTconstant.Line 104 uses
h-[calc(50vh-64px)]while the sheet height is controlled by theSHEET_HEIGHTconstant on Line 13. IfSHEET_HEIGHTchanges, this calc won't update, causing layout overflow or wasted space.Consider using an inline style for consistency:
Proposed fix
- <div className="h-[calc(50vh-64px)] overflow-y-auto px-6 py-6 space-y-8 text-white"> + <div className="overflow-y-auto px-6 py-6 space-y-8 text-white" style={{ height: `calc(${SHEET_HEIGHT} - 4rem)` }}>
89-147: Consider adding keyboard dismiss and focus management for the menu sheet.The half-screen sheet behaves like a modal overlay but:
- Pressing
Escapedoesn't close it.- Focus is not trapped — keyboard users can tab to elements behind the backdrop.
A minimal improvement would be adding an
Escapekey handler:Proposed fix for Escape key
+ /* Close sheet on Escape */ + useEffect(() => { + if (!open) return; + const onKey = (e: KeyboardEvent) => { + if (e.key === "Escape") setOpen(false); + }; + window.addEventListener("keydown", onKey); + return () => window.removeEventListener("keydown", onKey); + }, [open]);
1 participant
Closes #29
screen-capture (23).webm
@KanishkSogani Please review
Summary by CodeRabbit
New Features
Improvements