Active State Highlight – Underline in desktop view, side border in mo… #35
Conversation
WalkthroughNavbar.jsx refactors rendering for responsive behavior: new fixed gradient top bar, separate desktop and mobile navigation, active-state styles, a toggle-driven mobile panel that closes on item click, updated logo container and sizing, and revised layout/spacing classes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant N as Navbar
participant D as Desktop Nav
participant M as Mobile Panel
rect rgba(230,240,255,0.4)
note over N: Initial Render (responsive)
N->>D: Render horizontal nav (desktop >= breakpoint)
N-->>M: Do not render mobile panel (hidden)
end
alt Desktop navigation
U->>D: Click nav item
D->>N: setActive(link)
N-->>D: Re-render with active styles (text color + underline)
else Mobile navigation
U->>N: Tap menu toggle
N->>M: Open mobile panel (top-right)
U->>M: Tap nav item
M->>N: setActive(link)
N->>M: Close panel
N-->>N: Re-render with active styles
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 3
🧹 Nitpick comments (2)
src/components/Navbar.jsx (2)
6-7: Initialize active state fromnavLinksor URL hash.The hardcoded
"Home"assumes the first nav item is always "Home" and won't adapt ifnavLinkschanges. Consider initializing fromnavLinks[0].titleor deriving fromwindow.location.hash.Apply this diff:
- const [active, setActive] = useState("Home"); + const [active, setActive] = useState(navLinks[0]?.title || "Home");
10-14: Verify page content accommodates fixed navbar & improve logo alt text.
- The
fixed top-0navbar will overlap content. Ensure the page body has sufficient top padding (e.g.,pt-20or equivalent).- The logo alt text
"logo"is generic. Use a descriptive alternative like"Hoobank logo"for better accessibility.To improve accessibility, apply this diff:
- <img src={logo} alt="logo" className="w-[140px] h-[40px] object-contain" /> + <img src={logo} alt="Hoobank logo" className="w-[140px] h-[40px] object-contain" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Navbar.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Navbar.jsx (1)
src/constants/index.js (2)
navLinks(3-20)navLinks(3-20)
| <ul className="hidden sm:flex items-center gap-8"> | ||
| {navLinks.map((nav) => ( | ||
| <li key={nav.id}> | ||
| <a | ||
| href={`#${nav.id}`} | ||
| onClick={() => setActive(nav.title)} | ||
| className={`text-base font-medium transition-all duration-300 ${ | ||
| active === nav.title | ||
| ? "text-white border-b-2 border-pink-500" | ||
| : "text-gray-300 hover:text-white" | ||
| }`} | ||
| > | ||
| {nav.title} | ||
| </a> | ||
| </li> | ||
| ))} | ||
| </ul> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add keyboard focus styles for accessibility.
The desktop menu lacks visible focus indicators for keyboard navigation. Add focus-visible:outline or similar styling to improve accessibility for keyboard users.
Apply this diff:
className={`text-base font-medium transition-all duration-300 ${
active === nav.title
? "text-white border-b-2 border-pink-500"
- : "text-gray-300 hover:text-white"
+ : "text-gray-300 hover:text-white focus-visible:outline focus-visible:outline-2 focus-visible:outline-pink-500 focus-visible:outline-offset-2"
}`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ul className="hidden sm:flex items-center gap-8"> | |
| {navLinks.map((nav) => ( | |
| <li key={nav.id}> | |
| <a | |
| href={`#${nav.id}`} | |
| onClick={() => setActive(nav.title)} | |
| className={`text-base font-medium transition-all duration-300 ${ | |
| active === nav.title | |
| ? "text-white border-b-2 border-pink-500" | |
| : "text-gray-300 hover:text-white" | |
| }`} | |
| > | |
| {nav.title} | |
| </a> | |
| </li> | |
| ))} | |
| </ul> | |
| <ul className="hidden sm:flex items-center gap-8"> | |
| {navLinks.map((nav) => ( | |
| <li key={nav.id}> | |
| <a | |
| href={`#${nav.id}`} | |
| onClick={() => setActive(nav.title)} | |
| className={`text-base font-medium transition-all duration-300 ${ | |
| active === nav.title | |
| ? "text-white border-b-2 border-pink-500" | |
| : "text-gray-300 hover:text-white focus-visible:outline focus-visible:outline-2 focus-visible:outline-pink-500 focus-visible:outline-offset-2" | |
| }`} | |
| > | |
| {nav.title} | |
| </a> | |
| </li> | |
| ))} | |
| </ul> |
🤖 Prompt for AI Agents
In src/components/Navbar.jsx around lines 17 to 33, the desktop nav links lack
visible keyboard focus indicators; update the anchor className to include
focus-visible utilities (e.g., add focus-visible:outline or focus-visible:ring
classes, color and radius that match the design, and keep them alongside the
existing active/hover classes) so keyboard users see a clear focus ring; ensure
the focus styles do not conflict with the active state and apply to the <a>
element so tabbing shows a visible outline/ring.
| <div className="sm:hidden flex"> | ||
| <img | ||
| src={toggle ? close : menu} | ||
| alt="menu" | ||
| className="w-[28px] h-[28px] object-contain" | ||
| className="w-7 h-7 cursor-pointer" | ||
| onClick={() => setToggle(!toggle)} | ||
| /> |
There was a problem hiding this comment.
Use semantic button with ARIA attributes for menu toggle.
The toggle is currently an <img> with onClick, which is not keyboard-accessible or screen-reader-friendly. Wrap it in a <button> with appropriate ARIA attributes.
Apply this diff:
<div className="sm:hidden flex">
- <img
- src={toggle ? close : menu}
- alt="menu"
- className="w-7 h-7 cursor-pointer"
- onClick={() => setToggle(!toggle)}
- />
+ <button
+ onClick={() => setToggle(!toggle)}
+ aria-label={toggle ? "Close menu" : "Open menu"}
+ aria-expanded={toggle}
+ className="p-1"
+ >
+ <img
+ src={toggle ? close : menu}
+ alt=""
+ className="w-7 h-7"
+ />
+ </button>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="sm:hidden flex"> | |
| <img | |
| src={toggle ? close : menu} | |
| alt="menu" | |
| className="w-[28px] h-[28px] object-contain" | |
| className="w-7 h-7 cursor-pointer" | |
| onClick={() => setToggle(!toggle)} | |
| /> | |
| <div className="sm:hidden flex"> | |
| <button | |
| onClick={() => setToggle(!toggle)} | |
| aria-label={toggle ? "Close menu" : "Open menu"} | |
| aria-expanded={toggle} | |
| className="p-1" | |
| > | |
| <img | |
| src={toggle ? close : menu} | |
| alt="" | |
| className="w-7 h-7" | |
| /> | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
In src/components/Navbar.jsx around lines 36 to 42, the menu toggle is currently
an <img> with an onClick which is not keyboard-accessible or
screen-reader-friendly; replace the clickable image with a semantic <button
type="button"> that contains the image (or uses the image as decorative with
alt="" if the button has an accessible label), move the onClick handler to the
button, add ARIA attributes such as aria-expanded={toggle} and
aria-label="Toggle menu" (or a context-specific label), ensure the button
retains visual classes (or wrap the image in the button and keep sizing
classes), and ensure focus styles are preserved so keyboard users can operate
the toggle.
| {toggle && ( | ||
| <div className="sm:hidden absolute top-16 right-4 w-56 bg-gray-800 rounded-xl shadow-lg p-6 flex flex-col gap-4 transition-all duration-300"> | ||
| {navLinks.map((nav) => ( | ||
| <a | ||
| key={nav.id} | ||
| href={`#${nav.id}`} | ||
| onClick={() => { | ||
| setActive(nav.title); | ||
| setToggle(false); | ||
| }} | ||
| className={`text-lg font-medium ${ | ||
| active === nav.title | ||
| ? "text-white border-l-4 border-pink-500 pl-2" | ||
| : "text-gray-300 hover:text-white" | ||
| }`} | ||
| > | ||
| {nav.title} | ||
| </a> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Improve mobile menu accessibility and UX.
The mobile sidebar has several accessibility and UX gaps:
- No click-outside-to-close behavior
- No escape key handler
- No focus management (should focus first item when opened)
- Missing keyboard focus indicators on links
- No focus trap to prevent tabbing outside the menu
Consider adding:
useEffectwith click-outside listener- Escape key handler
- Focus management on mount/unmount
- Focus-visible styles similar to desktop menu
For keyboard focus styles, apply this diff:
className={`text-lg font-medium ${
active === nav.title
? "text-white border-l-4 border-pink-500 pl-2"
- : "text-gray-300 hover:text-white"
+ : "text-gray-300 hover:text-white focus-visible:outline focus-visible:outline-2 focus-visible:outline-pink-500 focus-visible:outline-offset-2"
}`}For click-outside and escape key handling, you'll need to add a useEffect hook. Would you like me to provide a complete implementation or open an issue to track this enhancement?
Verifying if src/components/Navbar.jsx includes existing click-outside, escape key, or focus management logic…
🏁 Script executed:
#!/bin/bash
rg -nP --type=jsx 'useEffect' -C3 src/components/Navbar.jsx
rg -nP --type=jsx 'addEventListener' -C3 src/components/Navbar.jsx
rg -nP --type=jsx 'onKeyDown' -C3 src/components/Navbar.jsx
rg -nP --type=jsx 'tabIndex' -C3 src/components/Navbar.jsx
rg -nP --type=jsx 'focus' -C3 src/components/Navbar.jsxLength of output: 139
Verifying file location and existing logic…
🏁 Script executed:
#!/bin/bash
find src -type f | grep Navbar.jsxLength of output: 25
Inspecting the file for existing hooks and handlers…
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/components/Navbar.jsx
rg -n 'useEffect\|addEventListener\|onKeyDown\|tabIndex\|focus' -C3 src/components/Navbar.jsxLength of output: 2221
Add dismissal and focus management for mobile sidebar
The mobile menu (src/components/Navbar.jsx lines 46–66) lacks:
- Click-outside and Escape key handlers to close the menu
- Focus trap and initial focus on the first link when opened
- Visible keyboard focus outlines on links
Implement:
- A
useEffectto attach/detach document click and keydown listeners - A focus-trap (or use refs/tabIndex) to contain focus inside the menu and move focus to the first link on open
focus-visibleoutline styles (e.g.,focus-visible:outline-2 focus-visible:outline-pink-500) on each menu link
🤖 Prompt for AI Agents
In src/components/Navbar.jsx around lines 46–66, add dismissal and focus
management: attach a useEffect that registers document click listener to detect
clicks outside the mobile menu container ref and a keydown listener to close on
Escape (cleanup both on unmount), use refs for the menu container and the first
link so when toggle becomes true you call focus() on the first link and save the
previously focused element to restore focus when closing, implement a simple
focus trap by listening for Tab/Shift+Tab inside the keydown handler and cycling
focus between the first and last focusable elements in the menu, and add
keyboard-visible outline classes (e.g., focus-visible:outline-2
focus-visible:outline-pink-500) to each menu link; ensure all listeners are
removed and focus restored when the menu is closed.
1 participant
…bile view.
Summary by CodeRabbit
New Features
Style