fix(ui): improve mobile navigation menu in Header.tsx#2217
Conversation
Summary by CodeRabbit
WalkthroughUpdated mobile navigation in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ 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. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/Header.tsx (2)
134-144: Meet 44×44 tap target and add ARIA/focus affordances for the mobile menu buttonIcon size alone doesn’t guarantee the 40–44 px minimum hit area (PR req. #1). Make the Button itself at least 44×44, keep icon size consistent to avoid layout shift, and expose state via ARIA. Also wire aria-controls to the drawer.
Apply:
- <Button - onPress={toggleMobileMenu} - className="bg-transparent text-slate-300 hover:bg-transparent hover:text-slate-100 focus:outline-none" - > - <span className="sr-only">Open main menu</span> - {mobileMenuOpen ? ( - <FontAwesomeIcon icon={faTimes} className="h-8 w-8" /> - ) : ( - <FontAwesomeIcon icon={faBars} className="h-6 w-6" /> - )} + <Button + onPress={toggleMobileMenu} + aria-label={mobileMenuOpen ? 'Close main menu' : 'Open main menu'} + aria-expanded={mobileMenuOpen} + aria-controls="mobile-menu-drawer" + className="h-11 w-11 shrink-0 rounded-md bg-transparent p-0 text-slate-300 hover:bg-transparent hover:text-slate-100 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-blue-500 focus-visible:ring-offset-2 focus-visible:ring-offset-owasp-blue dark:focus-visible:ring-offset-slate-800" + > + <span className="sr-only">{mobileMenuOpen ? 'Close main menu' : 'Open main menu'}</span> + {mobileMenuOpen ? ( + <FontAwesomeIcon icon={faTimes} className="h-6 w-6" /> + ) : ( + <FontAwesomeIcon icon={faBars} className="h-6 w-6" /> + )}And on the drawer container:
- <div + <div + id="mobile-menu-drawer" className={cn(
233-234: Brand capitalization: “GitHub”Update the label to use the correct casing.
- text="Star On Github" + text="Star on GitHub"
🧹 Nitpick comments (2)
frontend/src/components/Header.tsx (2)
189-194: Submenu spacing improvements look good; consider 44px target for submenu itemsNice use of gap/space-y and increased header padding. To better hit the 44px target on links themselves, consider bumping submenu item padding from py-2 to py-3 and adding focus-visible rings for keyboard users.
Example (for the Link at Line 199):
className={cn( 'block w-full px-4 py-3 text-left text-sm text-slate-700 transition-colors duration-150 ease-in-out first:rounded-t-md last:rounded-b-md hover:bg-slate-100 hover:text-slate-900 dark:text-slate-300 dark:hover:bg-slate-700 dark:hover:text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-blue-500 focus-visible:ring-offset-2 focus-visible:ring-offset-owasp-blue dark:focus-visible:ring-offset-slate-800', pathname === sub.href && 'bg-blue-50 font-medium text-blue-600 dark:bg-blue-900/20 dark:text-blue-200' )}
25-53: Optional: add Escape-to-close and body scroll lock when drawer is openImproves usability on iOS/Android and keyboard UX.
useEffect(() => { const handleResize = () => { if (window.innerWidth >= desktopViewMinWidth) setMobileMenuOpen(false) } const handleOutsideClick = (event: Event) => { /* unchanged */ } const handleKeyDown = (e: KeyboardEvent) => { if (e.key === 'Escape') setMobileMenuOpen(false) } // Lock scroll while open if (mobileMenuOpen) document.body.style.overflow = 'hidden' else document.body.style.overflow = '' window.addEventListener('resize', handleResize) window.addEventListener('click', handleOutsideClick) window.addEventListener('keydown', handleKeyDown) return () => { window.removeEventListener('resize', handleResize) window.removeEventListener('click', handleOutsideClick) window.removeEventListener('keydown', handleKeyDown) document.body.style.overflow = '' } }, [mobileMenuOpen])
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/src/components/Header.tsx(4 hunks)
🔇 Additional comments (1)
frontend/src/components/Header.tsx (1)
156-160: LGTM: improved spacing in the mobile drawer header blockThe added gap improves readability and separation.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/Header.tsx (1)
134-141: Fix invalid Tailwind class, meet 44px tap target, add focus-visible ring + ARIA to the toggle
focus:outline-hiddenisn’t a Tailwind class; usefocus:outline-none.- Issue requires min 40×40 tap target; set the Button to 44×44 (h-11 w-11) and keep icon size modest.
- Expose state to AT: add
aria-controls/aria-expandedand make the sr-only label dynamic.Apply:
- <Button - onPress={toggleMobileMenu} - className="bg-transparent text-slate-300 hover:bg-transparent hover:text-slate-100 focus:outline-hidden" - > - <span className="sr-only">Open main menu</span> + <Button + onPress={toggleMobileMenu} + aria-controls="mobile-menu" + aria-expanded={mobileMenuOpen} + className="h-11 w-11 bg-transparent text-slate-300 hover:bg-transparent hover:text-slate-100 focus:outline-none focus-visible:ring-2 focus-visible:ring-blue-500 focus-visible:ring-offset-2 focus-visible:ring-offset-owasp-blue dark:focus-visible:ring-offset-slate-800" + > + <span className="sr-only">{mobileMenuOpen ? 'Close main menu' : 'Open main menu'}</span>Outside this range, add an id to the panel so
aria-controlsresolves:<div id="mobile-menu" className={cn(/* existing classes */)}>
♻️ Duplicate comments (1)
frontend/src/components/Header.tsx (1)
215-217: Adopt full-width 44px targets and focus-visible ring on mobile links (and thanks for fixing the dark:hover typo)Previous comment about
dark:hover::text-whiteis resolved. To further improve tap targets and keyboard focus:- className={cn( - 'navlink dark:hover:text-white block px-3 py-2 text-slate-700 transition duration-150 ease-in-out hover:bg-slate-100 hover:text-slate-900 dark:text-slate-300 dark:hover:bg-slate-700', + className={cn( + 'navlink block w-full h-11 px-4 text-left text-slate-700 transition-colors duration-150 ease-in-out hover:bg-slate-100 hover:text-slate-900 dark:text-slate-300 dark:hover:bg-slate-700 dark:hover:text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-blue-500 focus-visible:ring-offset-2 focus-visible:ring-offset-owasp-blue dark:focus-visible:ring-offset-slate-800', pathname === link.href && 'font-bold text-blue-800 dark:text-white' )} + aria-current={pathname === link.href ? 'page' : undefined}
🧹 Nitpick comments (2)
frontend/src/components/Header.tsx (2)
195-206: Expose active state to screen readers on submenu itemsStyle already reflects active route; mirror it with
aria-currentfor AT parity.- <Link + <Link key={i} href={sub.href || '/'} + aria-current={pathname === sub.href ? 'page' : undefined}
233-234: Typo: “Github” → “GitHub”Product name casing.
- text="Star On Github" + text="Star on GitHub"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/src/components/Header.tsx(4 hunks)
🔇 Additional comments (2)
frontend/src/components/Header.tsx (2)
156-156: LGTM: Increased spacing around the mobile header/logo
gap-5improves breathing room and touch ergonomics.
189-194: LGTM: Mobile submenu spacing improvements
gap-2,py-3, andspace-y-4make items more scannable and tappable.
|
* fix(ui): fixed usermenu styling on mobile screens * fixed syntax issue * Update styling --------- Co-authored-by: Kate <kate@kgthreads.com>
ishanBahuguna
deleted the
feature/improve-user-navigation-menu-on-small-screens
branch
2 participants
Proposed change
Resolves #2066
Issues not addressed due to design constraints:
Before: no effects
After:
owasp.mp4
Checklist
make check-testlocally; all checks and tests passed.