fix: make header responsive on small screens#102
Conversation
|
@zaibamachhaliya is attempting to deploy a commit to the niharika-mente's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthrough
ChangesNavLinks/Navbar Refactor
Config Reformatting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
Hi @maintainer PR is ready. Please review it. |
![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
🤖 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 `@components/NavLinks.tsx`:
- Around line 25-33: The Link components in the navigation lack aria-current
attributes to communicate active state to screen readers, and the menu toggle
button (around line 46) is missing aria-expanded and aria-controls attributes to
expose its expanded/collapsed state. Add aria-current="page" to the Link
component when pathname === link.href to mark the currently active link for
assistive technology. Additionally, add aria-expanded={open} (or appropriate
boolean state) and aria-controls to reference the menu element ID on the toggle
button to properly communicate the menu's open/closed state to screen readers.
In `@package.json`:
- Around line 14-22: Remove the `@next/swc-win32-x64-msvc` dependency from
package.json as Next.js manages native SWC binaries automatically, and manually
declaring them causes dependency conflicts. Additionally, update the
eslint-config-next version from 16.1.3 to align with the Next.js 15.1.0 version
being used (change it to a compatible 15.x.x version) to resolve the major
version incompatibility that will cause linting to break.
🪄 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: CHILL
Plan: Pro
Run ID: c770eac9-b7b4-4354-a3de-75d7dd44ae07
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (5)
components/NavLinks.tsxcomponents/Navbar.tsxnext.config.tspackage.jsontsconfig.json
|
@zaibamachhaliya, first resolve all the requested changes by bots! |
TarunyaProgrammer
left a comment
TarunyaProgrammer
left a comment
There was a problem hiding this comment.
Thanks for working on the responsive header. The mobile menu implementation and navbar cleanup look good overall, but I noticed a few issues that should be addressed before merging:
-
The new links array changes some existing routes:
Create Event→/events/create(currently/create-event)My Bookings→/bookings(currently/my-bookings)
These appear to be regressions and may lead to broken navigation.
-
The desktop navigation no longer includes
aria-currenton active links, and the mobile menu button is missingaria-expanded/aria-controls. This is a small accessibility regression from the current implementation. -
package.json,next.config.ts, andtsconfig.jsoncontain unrelated changes that don't seem necessary for a responsive-header fix. It would be better to either explain them in the PR or keep this PR focused on the navbar changes only.
Once these are addressed, the responsive navigation changes look good.
|
I have addressed the requested changes:
Could you please review the updated changes? Thank you! |
TarunyaProgrammer
left a comment
TarunyaProgrammer
left a comment
There was a problem hiding this comment.
The responsive navigation implementation looks good overall, and the accessibility improvements (aria-current, aria-expanded, aria-controls) are a nice addition. However, there is one blocking issue that needs to be addressed before this can be merged.
The PR downgrades next from 16.1.3 to ^15.1.0 and removes cacheComponents: true from next.config.ts. The project currently uses the 'use cache' directive in components/EventDetails.tsx, which requires cacheComponents and Next.js 16. As a result, npm run build fails with:
To use "use cache", please enable the feature flag `cacheComponents` in your Next.js config.
Additionally, the dependency versions become inconsistent after the downgrade (next@15.x alongside eslint-config-next@16.1.3 and @next/swc-win32-x64-msvc@16.1.3).
Please revert the Next.js downgrade and restore:
cacheComponents: truein next.config.ts, while keeping the responsive navbar changes. Once the build passes again, this should be good to go.
|
I have addressed all the requested changes:
I have pushed the latest updates to the branch. Could you please review the PR again when you have a chance? Thank you for your time and feedback! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/NavLinks.tsx (1)
47-79: ⚡ Quick winConsider adding keyboard and click-outside handlers for better UX.
The mobile menu implementation works well, but could be enhanced with two common patterns:
- Escape key to close: Pressing
Escapeshould close the menu for keyboard users- Click outside to close: Clicking outside the menu should dismiss it
These are optional enhancements that improve accessibility and user experience.
💡 Example implementation
// Add useEffect for keyboard handling useEffect(() => { if (!open) return; const handleEscape = (e: KeyboardEvent) => { if (e.key === 'Escape') setOpen(false); }; document.addEventListener('keydown', handleEscape); return () => document.removeEventListener('keydown', handleEscape); }, [open]); // Add ref and click-outside handler const menuRef = useRef<HTMLDivElement>(null); useEffect(() => { if (!open) return; const handleClickOutside = (e: MouseEvent) => { if (menuRef.current && !menuRef.current.contains(e.target as Node)) { setOpen(false); } }; document.addEventListener('mousedown', handleClickOutside); return () => document.removeEventListener('mousedown', handleClickOutside); }, [open]); // Add ref to mobile menu div <div ref={menuRef} id="mobile-menu" // ... rest of props >🤖 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 `@components/NavLinks.tsx` around lines 47 - 79, Add keyboard and click-outside event handlers to the NavLinks component to improve mobile menu accessibility. First, create a useRef for the mobile menu div and attach it to the element with id="mobile-menu". Then add a useEffect hook that listens for the Escape key when the menu is open and calls setOpen(false) to close it, ensuring proper cleanup of the event listener. Finally, add another useEffect hook that detects clicks outside the menu element and closes the menu by calling setOpen(false) when a user clicks outside the referenced menu area, also with proper event listener cleanup.
🤖 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 `@components/NavLinks.tsx`:
- Line 28: Remove the `onClick={() => setOpen(false)}` handler from the desktop
navigation links in the NavLinks component since the `open` state only controls
mobile menu visibility and the desktop menu is always visible at the `md`
breakpoint and above. Desktop links should not trigger mobile menu state
changes. Keep the onClick handler only on mobile menu elements where it is
actually needed to close the menu when a link is clicked.
---
Nitpick comments:
In `@components/NavLinks.tsx`:
- Around line 47-79: Add keyboard and click-outside event handlers to the
NavLinks component to improve mobile menu accessibility. First, create a useRef
for the mobile menu div and attach it to the element with id="mobile-menu". Then
add a useEffect hook that listens for the Escape key when the menu is open and
calls setOpen(false) to close it, ensuring proper cleanup of the event listener.
Finally, add another useEffect hook that detects clicks outside the menu element
and closes the menu by calling setOpen(false) when a user clicks outside the
referenced menu area, also with proper event listener cleanup.
🪄 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: CHILL
Plan: Pro
Run ID: 97a47488-be6b-4368-a715-75a2dfc39ebc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (2)
components/NavLinks.tsxnext.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- next.config.ts
3 participants
Description
This PR fixes the header responsiveness issue on small screen devices.
Changes Made
Tested On
Fixes:#99
Summary by CodeRabbit
New Features
Improvements