Fix mobile sidebar overlapping header buttons #27
Conversation
📝 WalkthroughWalkthroughMobile menu animation and positioning updated: vertical translate transitions replaced height-based transitions; container changed from normal flow to fixed positioning with full width, top offset, z-index, and dark background color. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 0
🧹 Nitpick comments (3)
frontend/src/components/Navbar.jsx (3)
253-257: Consider adding body scroll lock and click-outside-to-close functionality.The fixed positioning and animation work correctly, but the mobile menu UX could be improved:
- Body scroll lock: When the menu is open, users can still scroll the page content behind it, which can be disorienting on mobile devices.
- Click-outside to close: There's no backdrop overlay or click-outside handler to dismiss the menu, which is a common mobile UX pattern.
- Visual separation: Consider adding a semi-transparent backdrop to visually separate the menu from the page content.
🔎 Proposed improvements
Add body scroll lock effect:
+ // Lock body scroll when mobile menu is open + useEffect(() => { + if (isMobileMenuOpen) { + document.body.style.overflow = 'hidden'; + } else { + document.body.style.overflow = 'unset'; + } + return () => { + document.body.style.overflow = 'unset'; + }; + }, [isMobileMenuOpen]);Add a backdrop overlay with click-to-close:
<AnimatePresence> {isMobileMenuOpen && ( + <> + {/* Backdrop */} + <motion.div + initial={{ opacity: 0 }} + animate={{ opacity: 1 }} + exit={{ opacity: 0 }} + onClick={closeMobileMenu} + className="fixed inset-0 bg-black/50 z-30" + /> + {/* Mobile Menu */} <motion.div initial={{ opacity: 0, y: -10 }} animate={{ opacity: 1, y: 0 }} exit={{ opacity: 0, y: -10 }} transition={{ duration: 0.3 }} className="md:hidden fixed top-24 left-0 w-full z-40 bg-[#161920] overflow-hidden" >
259-259: Remove unnecessary empty line.This line appears to be marked as modified but contains no meaningful content. Consider removing it to keep the code clean.
234-245: Add accessibility attributes to mobile menu button.The mobile menu toggle button is missing ARIA attributes that help screen reader users understand its purpose and state.
🔎 Proposed accessibility improvements
<motion.button whileHover={{ scale: 1.1 }} whileTap={{ scale: 0.9 }} onClick={toggleMobileMenu} + aria-expanded={isMobileMenuOpen} + aria-label={isMobileMenuOpen ? "Close menu" : "Open menu"} className="text-gray-300 hover:text-white focus:outline-none" > {isMobileMenuOpen ? ( <CloseIcon className="h-6 w-6" /> ) : ( <MenuIcon className="h-6 w-6" /> )} </motion.button>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Navbar.jsx
🔇 Additional comments (1)
frontend/src/components/Navbar.jsx (1)
34-45: Verify effect dependency array matches the condition logic.The effect checks
addresson line 35 but the dependency array includesisConnectedinstead ofaddress. This might be intentional (to trigger only on connection status change), but it creates a potential inconsistency if the address changes withoutisConnectedchanging.Consider whether
addressshould be in the dependency array:- }, [isConnected, hasConnected, navigate, location.pathname]); + }, [address, hasConnected, navigate, location.pathname]);Or if the current behavior is intentional, consider checking
isConnectedinstead ofaddressfor clarity:- if (address && !hasConnected && location.pathname === "/") { + if (isConnected && !hasConnected && location.pathname === "/") {
|
Thanks for the review! The core goal of this PR was to resolve the mobile sidebar overlapping the header buttons, which has been addressed by adjusting the mobile menu positioning below the fixed navbar. The additional suggestions around scroll locking, backdrop overlay, and accessibility are great improvements. I kept this PR intentionally scoped to the reported issue to avoid expanding its scope, but I’d be happy to incorporate these enhancements if the maintainers prefer them in this PR or as a follow-up. Please let me know how you’d like to proceed. |
|
@nishi240931 for UI related changes, please attach a screenshot or a short video showing the updates you've made. |
|
@nishi240931 We will fix project_id problem asap! |
|
Thanks for the review! The fix is scoped to the mobile menu in: Specifically, I updated the <motion.div> inside the “Mobile Menu” section to:
This prevents the mobile sidebar from expanding inside the fixed navbar and resolves the header overlap issue on small screens. Note: The page content appears blank locally due to the known WalletConnect projectId issue (already acknowledged by maintainers). Once that is resolved, the fix can be visually verified in mobile view. 
|
|
@nishi240931 ,Project id problem has been resolved you can work further |
|
Thank you for the update! I’ll continue working on the project. |
|
Hi , The mobile menu no longer overlaps the header, and the navbar remains visible in mobile view. Thanks! |
2 participants
Problem
On mobile screens, the fixed navbar caused the mobile menu to expand inside it,
leading to overlap with header buttons.
Solution
Updated the mobile menu to render as a fixed overlay positioned below the navbar
height, preventing overlap while keeping desktop behavior unchanged.
Notes
Local runtime requires a WalletConnect projectId, so the fix was validated via
responsive layout inspection.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.