Improve navbar hover and active state visibility#4379
Improve navbar hover and active state visibility#4379Awanthiy wants to merge 3 commits intoOWASP:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughReplaced className composition for non-dropdown desktop navigation links in Header.tsx to new Tailwind classes (hover, active, transition) and made Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 3 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![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
🧹 Nitpick comments (1)
frontend/src/components/Header.tsx (1)
97-117: Desktop and mobile navigation styling are now inconsistent.The PR objective states "consistent hover and active behavior across both light and dark themes." The desktop links now use lime/blue active states (
text-lime-700 dark:text-blue-400), but the mobile menu at lines 221-231 still uses the old blue-based styling (font-bold text-blue-800 dark:text-white). Consider updating the mobile menu to match for visual consistency.♻️ Suggested mobile menu update (lines 224-226)
className={cn( - 'navlink 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 dark:hover:text-white', - pathname === link.href && 'font-bold text-blue-800 dark:text-white' + 'navlink block px-3 py-2 text-slate-700 transition duration-150 ease-in-out hover:bg-slate-100 hover:text-lime-600 dark:text-slate-300 dark:hover:bg-slate-700 dark:hover:text-blue-400', + pathname === link.href && 'font-semibold text-lime-700 dark:text-blue-400' )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Header.tsx` around lines 97 - 117, Update the mobile menu link classes to match the desktop link hover/active behavior: replace the old classes (e.g., "font-bold text-blue-800 dark:text-white") used in the mobile menu Link rendering with the same class pattern used in the Header desktop Link (the cn(...) call that yields "text-lime-700 dark:text-blue-400" and "font-semibold" for active state and the hover variants). Locate the mobile menu's JSX that maps over links (the mobile Link elements in the Header component) and apply the same cn(...) class logic or equivalent class names so hover and active styling are consistent across light/dark themes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/Header.tsx`:
- Line 114: Header.tsx currently sets aria-current="page" unconditionally on nav
links; change it to only include aria-current="page" when the link is the active
route. Fix by computing an isActive boolean (e.g., using react-router's
useLocation or NavLink's isActive callback) and render aria-current={isActive ?
"page" : undefined} (or omit the attribute when false) on the link element in
Header (the element that currently contains aria-current="page"). This ensures
only the current page link has aria-current set.
---
Nitpick comments:
In `@frontend/src/components/Header.tsx`:
- Around line 97-117: Update the mobile menu link classes to match the desktop
link hover/active behavior: replace the old classes (e.g., "font-bold
text-blue-800 dark:text-white") used in the mobile menu Link rendering with the
same class pattern used in the Header desktop Link (the cn(...) call that yields
"text-lime-700 dark:text-blue-400" and "font-semibold" for active state and the
hover variants). Locate the mobile menu's JSX that maps over links (the mobile
Link elements in the Header component) and apply the same cn(...) class logic or
equivalent class names so hover and active styling are consistent across
light/dark themes.
🪄 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: efea18bb-6d51-4d56-80e2-ad817ebe9b0f
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
frontend/src/components/Header.tsx
frontend/package-lock.json
Outdated
There was a problem hiding this comment.
Hi @Awanthiy, thanks for working on this.
Did you add any pkg?
I don't think we need it just for improving the hover effect for the navbar...
It's 25k+ lines here
There was a problem hiding this comment.
Thanks for pointing that out. I accidentally included the lockfile changes generated locally by npm. I’ve now removed them so the PR only contains the intended navbar updates.
There was a problem hiding this comment.
Its recommended to run make check-test locally before pushing your code.
There was a problem hiding this comment.
Thanks for pointing that out @hassaansaleem28 ! Moreover, it's actually required 👍🏼
Awanthiy
dismissed stale reviews from coderabbitai[bot] and cubic-dev-ai[bot]
via
cfa7f0c
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@Awanthiy thank you for working on this.
I'm not sure I like the lime color for hover color to be honest 🤔 Do you have any other options that would work with out current color scheme in mind?
If so, could you first attach screenshots to this PR so that we can compare a couple of options? Ideally that would be added to the issue, but since we are already on the PR stage - let's do it here.
Also, please run make check-test - this is required for all PRs. Currently both linter checks and tests are failing.
| href={link.href || '/'} | ||
| className={cn( | ||
| ` | ||
| px-3 py-2 text-sm font-medium |
There was a problem hiding this comment.
Not sure why you wanted to make the text smaller... but can we keep it as it was before? 14px is too small in my opinion.
| px-3 py-2 text-sm font-medium | |
| px-3 py-2 font-medium |
3 participants
Proposed change
Resolves #4364
Improved navbar hover and active states to enhance visibility and usability in both light and dark modes.
Changes made
Notes
The light mode color can be further refined based on feedback.
Checklist
make check-testlocally