Hambuger drop-down has no background coverage causing overlapping of homepage and drop down component#30
Hambuger drop-down has no background coverage causing overlapping of homepage and drop down component#30aniket866 wants to merge 3 commits into
Conversation
WalkthroughNavbar is refactored to a simplified mobile half-sheet overlay controlled by ChangesNavbar Component Redesign
Sequence Diagram(s)sequenceDiagram
actor User
participant Navbar
participant MobileSheet
participant Backdrop
participant Body as document.body
participant ConnectButton
participant WalletConfig
User->>Navbar: Tap hamburger menu
Navbar->>Navbar: set open = true
Navbar->>Backdrop: render backdrop
Navbar->>Body: set overflow = 'hidden'
Backdrop->>MobileSheet: show sheet overlay
rect rgba(100, 200, 255, 0.5)
User->>MobileSheet: view links / wallet row
MobileSheet->>ConnectButton: render ConnectButton
User->>ConnectButton: interact (connect)
User->>MobileSheet: tap wallet config (if unauthenticated)
MobileSheet->>WalletConfig: call toggleWalletConfig()
end
User->>Backdrop: tap close or action completes
Navbar->>Navbar: set open = false
Navbar->>Backdrop: hide backdrop
Navbar->>Body: set overflow = ''
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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]);
There was a problem hiding this comment.
Pull request overview
This PR updates the Navbar mobile navigation to prevent the hamburger menu from visually overlapping the homepage by rendering a full-viewport menu overlay and updating navbar styling based on scroll state.
Changes:
- Reworked navbar scroll-background behavior (landing page vs non-landing pages).
- Replaced the previous mobile dropdown with a full-screen mobile menu overlay.
- Added body scroll locking when the mobile menu is open.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
hey @aniket866 can you please resolve copilot comments. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/Navbar.tsx (1)
97-97: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExternalize user-facing menu strings to i18n resources.
The new inline labels should be moved to translation/resource files instead of being hardcoded in the component.
As per coding guidelines, "Internationalization: User-visible strings should be externalized to resource files (i18n)".
Also applies to: 111-111, 117-117, 124-124, 137-139
🤖 Prompt for 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. In `@web/src/components/Navbar.tsx` at line 97, The Navbar component contains hardcoded user-facing strings (e.g., the <span> "Menu" and other labels mentioned) that must be moved into i18n resource files; update Navbar.tsx to import and use your app's translation helper (e.g., useTranslation/t or similar) and replace the inline strings with translation keys (e.g., "navbar.menu", "navbar.X") referenced from the locale JSONs, then add those keys/values to the i18n resource files for all supported locales; ensure the component falls back to the key or a default if the translation is missing.
🤖 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 `@web/src/components/Navbar.tsx`:
- Line 13: SHEET_HEIGHT currently uses "100vh" which can cause mobile chrome
layout glitches; update the SHEET_HEIGHT constant and any related content height
calculations (e.g., the calc that subtracts header/footer at the content usage
around the symbol referencing the content calc near line 104) to use dynamic
viewport units—prefer "100dvh" with a safe fallback to "100svh" or "100vh" if
necessary—so replace occurrences of "100vh" and "calc(100vh - ...)" with
"100dvh" (or "calc(100dvh - ...)" plus a fallback) so the sheet and its content
consistently cover the full viewport on mobile.
- Line 92: The full-screen mobile menu overlay in Navbar.tsx is missing proper
dialog semantics and Escape-key handling; update the overlay element (the div
with className "fixed inset-0 z-50 md:hidden") to include role="dialog" and
aria-modal="true" plus either aria-label or aria-labelledby referencing the menu
title, add an accessible close control (a visually-hidden label or aria-label on
the close button) and wire Escape-to-close by adding a keydown listener in the
Navbar component (or the hook that manages mobileMenuOpen) which calls the
existing close handler (e.g., setMobileMenuOpen(false) or the closeMenu
function) and ensure the listener is cleaned up in useEffect; also apply the
same semantic attributes to the other overlay instance referenced around lines
98–100 so both mobile overlays are accessible.
---
Outside diff comments:
In `@web/src/components/Navbar.tsx`:
- Line 97: The Navbar component contains hardcoded user-facing strings (e.g.,
the <span> "Menu" and other labels mentioned) that must be moved into i18n
resource files; update Navbar.tsx to import and use your app's translation
helper (e.g., useTranslation/t or similar) and replace the inline strings with
translation keys (e.g., "navbar.menu", "navbar.X") referenced from the locale
JSONs, then add those keys/values to the i18n resource files for all supported
locales; ensure the component falls back to the key or a default if the
translation is missing.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e5b2978-6eb8-4a47-a518-0b6fe59d6e4c
📒 Files selected for processing (1)
web/src/components/Navbar.tsx
| import VouchMeLogo from "../image/VouchMeLogo.png"; | ||
|
|
||
| const NAVY_BG = "#0B1C2D"; | ||
| const SHEET_HEIGHT = "100vh"; |
There was a problem hiding this comment.
Use dynamic viewport units for reliable full-screen coverage on mobile.
Using 100vh here can still produce visible layout glitches with mobile browser UI chrome. Switch both the sheet height and content calc to 100dvh (or svh fallback) to keep the overlay coverage consistent.
Suggested patch
-const SHEET_HEIGHT = "100vh";
+const SHEET_HEIGHT = "100dvh";
...
- <div className="h-[calc(100vh-64px)] overflow-y-auto px-6 py-6 space-y-8 text-white">
+ <div className="h-[calc(100dvh-64px)] overflow-y-auto px-6 py-6 space-y-8 text-white">Also applies to: 104-104
🤖 Prompt for 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.
In `@web/src/components/Navbar.tsx` at line 13, SHEET_HEIGHT currently uses
"100vh" which can cause mobile chrome layout glitches; update the SHEET_HEIGHT
constant and any related content height calculations (e.g., the calc that
subtracts header/footer at the content usage around the symbol referencing the
content calc near line 104) to use dynamic viewport units—prefer "100dvh" with a
safe fallback to "100svh" or "100vh" if necessary—so replace occurrences of
"100vh" and "calc(100vh - ...)" with "100dvh" (or "calc(100dvh - ...)" plus a
fallback) so the sheet and its content consistently cover the full viewport on
mobile.
| {/* FULL-SCREEN MENU */} | ||
| {open && ( | ||
| <div | ||
| className="fixed inset-0 z-50 md:hidden" |
There was a problem hiding this comment.
Add modal semantics and keyboard-close support for the full-screen menu.
This overlay behaves as a modal, but it lacks dialog semantics and robust keyboard handling. Add role="dialog", aria-modal, an accessible close label, and Escape-to-close.
Suggested patch
+ useEffect(() => {
+ if (!open) return;
+ const onKeyDown = (e: KeyboardEvent) => {
+ if (e.key === "Escape") setOpen(false);
+ };
+ window.addEventListener("keydown", onKeyDown);
+ return () => window.removeEventListener("keydown", onKeyDown);
+ }, [open]);
...
- <div
- className="fixed inset-0 z-50 md:hidden"
+ <div
+ role="dialog"
+ aria-modal="true"
+ aria-label="Navigation menu"
+ className="fixed inset-0 z-50 md:hidden"
style={{ height: SHEET_HEIGHT, backgroundColor: NAVY_BG }}
>
...
- <button onClick={() => setOpen(false)} className="text-white">
+ <button
+ onClick={() => setOpen(false)}
+ className="text-white"
+ aria-label="Close menu"
+ >Also applies to: 98-100
🤖 Prompt for 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.
In `@web/src/components/Navbar.tsx` at line 92, The full-screen mobile menu
overlay in Navbar.tsx is missing proper dialog semantics and Escape-key
handling; update the overlay element (the div with className "fixed inset-0 z-50
md:hidden") to include role="dialog" and aria-modal="true" plus either
aria-label or aria-labelledby referencing the menu title, add an accessible
close control (a visually-hidden label or aria-label on the close button) and
wire Escape-to-close by adding a keydown listener in the Navbar component (or
the hook that manages mobileMenuOpen) which calls the existing close handler
(e.g., setMobileMenuOpen(false) or the closeMenu function) and ensure the
listener is cleaned up in useEffect; also apply the same semantic attributes to
the other overlay instance referenced around lines 98–100 so both mobile
overlays are accessible.
Closes #29
screen-capture (23).webm
@KanishkSogani Please review
Summary by CodeRabbit
New Features
Improvements