-
Notifications
You must be signed in to change notification settings - Fork 65
✨ Enhance Navbar Styling with Better Headings and Hover Effects #209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughNavbar refactored to use a shared navLinks array, adds route-aware active styling via useLocation, updates desktop/mobile visuals and animations (blurred background, gradient underlines, hover effects), moves/enhances theme toggle, and adds a subtle drop-shadow Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router as Router
participant Navbar as Navbar
participant Theme as ThemeContext
Router->>Navbar: render with current location
Navbar->>Navbar: build nav items from navLinks
Navbar->>Navbar: compare location.pathname → apply active styles
Note right of Navbar: gradient underline + hover animation
User->>Navbar: click theme toggle
Navbar->>Theme: toggleTheme()
Theme-->>Navbar: updated theme
Navbar-->>User: re-render with new styles
User->>Navbar: toggle mobile menu
Navbar->>Navbar: isOpen = !isOpen (animate max-h / opacity)
Navbar-->>User: show/hide mobile panel with mapped links
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Thank you @janvis11 for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/components/Navbar.tsx (7)
70-91
: Add ARIA and restore visible focus for the hamburger button.Current button hides outlines and lacks name/state; hurts keyboard/screen-reader users.
- <button - onClick={() => setIsOpen(!isOpen)} - className="relative w-8 h-8 flex flex-col space-y-[5px] items-center group focus:outline-none" - > + <button + type="button" + onClick={() => setIsOpen(!isOpen)} + aria-label={isOpen ? "Close navigation menu" : "Open navigation menu"} + aria-controls="mobile-nav" + aria-expanded={isOpen} + className="relative w-8 h-8 flex flex-col space-y-[5px] items-center group focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-indigo-400 focus-visible:ring-offset-2" + >
95-101
: Connect aria-controls and label the mobile region.Tie the toggle to the panel and add a region label.
- <div + <div + id="mobile-nav" + role="region" + aria-label="Mobile navigation" className={`md:hidden overflow-hidden transition-all duration-500 ${ isOpen ? "max-h-96 opacity-100" : "max-h-0 opacity-0" }`} > - <div className="bg-white/90 dark:bg-gray-800/90 backdrop-blur-md text-black dark:text-white border-t border-gray-200 dark:border-gray-700 px-6 py-4 space-y-4"> + <div className="bg-white/90 dark:bg-gray-800/90 backdrop-blur-md text-black dark:text-white border-t border-gray-200 dark:border-gray-700 px-6 py-4 space-y-4">
56-66
: Theme toggle needs accessible name/state and focus style.- <button - onClick={toggleTheme} - className="ml-4 p-2 rounded-full border border-gray-400 dark:border-gray-600 hover:scale-110 hover:border-indigo-400 transition-all duration-500 transform hover:rotate-12" - > + <button + type="button" + onClick={toggleTheme} + aria-pressed={mode === "dark"} + aria-label={mode === "dark" ? "Switch to light mode" : "Switch to dark mode"} + className="ml-4 p-2 rounded-full border border-gray-400 dark:border-gray-600 hover:scale-110 hover:border-indigo-400 transition-all duration-500 transform hover:rotate-12 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-indigo-400 focus-visible:ring-offset-2 motion-reduce:transform-none" + >
116-124
: Mobile theme toggle: add ARIA pressed/name for parity with desktop.- <button + <button + type="button" onClick={() => { toggleTheme(); setIsOpen(false); }} - className="w-full text-left text-sm font-semibold px-3 py-2 rounded-lg border border-gray-500 hover:border-indigo-400 hover:text-indigo-400 dark:hover:text-indigo-300 transition duration-300" + aria-pressed={mode === "dark"} + aria-label={mode === "dark" ? "Switch to light mode" : "Switch to dark mode"} + className="w-full text-left text-sm font-semibold px-3 py-2 rounded-lg border border-gray-500 hover:border-indigo-400 hover:text-indigo-400 dark:hover:text-indigo-300 transition duration-300 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-indigo-400 focus-visible:ring-offset-2 motion-reduce:transition-none" >
96-99
: Avoid fixed 24rem max-height; safer for future link growth.Use viewport-based max height and allow scrolling to prevent clipping.
- className={`md:hidden overflow-hidden transition-all duration-500 ${ - isOpen ? "max-h-96 opacity-100" : "max-h-0 opacity-0" - }`} + className={`md:hidden overflow-hidden transition-[max-height,opacity] duration-500 ${ + isOpen ? "max-h-[60vh] opacity-100" : "max-h-0 opacity-0" + }`}Also consider adding "overflow-y-auto" to the inner panel if links may exceed the height.
43-49
: Add visible focus styles on nav links.Improves keyboard navigation without changing hover visuals.
- className={`relative text-lg font-medium px-3 py-2 rounded-lg transition-all duration-300 group + className={`relative text-lg font-medium px-3 py-2 rounded-lg transition-all duration-300 group focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-indigo-400And mirror the same for the mobile link className base string.
Also applies to: 106-112
11-11
: Don’t fail silently when ThemeContext is missing.Returning null hides primary navigation; consider a defensive hook that throws with a clear message or a minimal fallback UI.
If preferred, I can draft a small
useTheme()
wrapper that enforces non-null context and centralizes the null check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Navbar.tsx
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Navbar.tsx (1)
src/context/ThemeContext.tsx (1)
ThemeContext
(10-10)
🔇 Additional comments (1)
src/components/Navbar.tsx (1)
15-20
: Confirm /about & /contact presence; hoist navLinks
- src/Routes/Router.tsx (lines ~14–21) includes /about and /contact — confirm their omission from the navbar is intentional.
- Move the const navLinks (src/components/Navbar.tsx:15–20) outside the Navbar component or wrap it in useMemo([]) to avoid reallocating the array on every render.
@@ -1,62 +1,68 @@ | |||
import { Link } from "react-router-dom"; | |||
import { Link, useLocation } from "react-router-dom"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use NavLink to simplify active state and improve a11y (aria-current).
You can drop manual pathname checks and get correct exact/partial matching + aria-current for free.
- import { Link, useLocation } from "react-router-dom";
+ import { Link, NavLink } from "react-router-dom";
@@
- const location = useLocation();
@@
- {navLinks.map((link) => (
- <Link
- key={link.to}
- to={link.to}
- className={`relative text-lg font-medium px-3 py-2 rounded-lg transition-all duration-300 group
- ${
- location.pathname === link.to
- ? "text-indigo-500 dark:text-indigo-400 font-semibold bg-indigo-50 dark:bg-indigo-900"
- : "hover:text-indigo-400 dark:hover:text-indigo-300 hover:bg-indigo-100 dark:hover:bg-gray-800"
- }`}
- >
+ {navLinks.map((link) => (
+ <NavLink
+ key={link.to}
+ to={link.to}
+ end={link.to === "/"}
+ className={({ isActive }) =>
+ `relative text-lg font-medium px-3 py-2 rounded-lg transition-all duration-300 group ${
+ isActive
+ ? "text-indigo-500 dark:text-indigo-400 font-semibold bg-indigo-50 dark:bg-indigo-900"
+ : "hover:text-indigo-400 dark:hover:text-indigo-300 hover:bg-indigo-100 dark:hover:bg-gray-800"
+ }`
+ }
+ >
@@
- </Link>
+ </NavLink>
@@
- {navLinks.map((link) => (
- <Link
- key={link.to}
- to={link.to}
- onClick={() => setIsOpen(false)}
- className={`block text-lg font-medium px-3 py-2 rounded-lg transition-all duration-300
- ${
- location.pathname === link.to
- ? "text-indigo-500 dark:text-indigo-400 font-semibold bg-indigo-50 dark:bg-indigo-900"
- : "hover:text-indigo-400 dark:hover:text-indigo-300 hover:bg-indigo-100 dark:hover:bg-gray-800"
- }`}
- >
+ {navLinks.map((link) => (
+ <NavLink
+ key={link.to}
+ to={link.to}
+ end={link.to === "/"}
+ onClick={() => setIsOpen(false)}
+ className={({ isActive }) =>
+ `block text-lg font-medium px-3 py-2 rounded-lg transition-all duration-300 ${
+ isActive
+ ? "text-indigo-500 dark:text-indigo-400 font-semibold bg-indigo-50 dark:bg-indigo-900"
+ : "hover:text-indigo-400 dark:hover:text-indigo-300 hover:bg-indigo-100 dark:hover:bg-gray-800"
+ }`
+ }
+ >
@@
- </Link>
+ </NavLink>
Also applies to: 9-9, 39-54, 101-115
🤖 Prompt for AI Agents
In src/components/Navbar.tsx around lines 1 (and also apply to ranges 9-9,
39-54, 101-115), you are using Link and manual pathname checks to set an active
state and ARIA attributes; replace Link with React Router's NavLink (update the
import) and remove the manual pathname comparisons, relying on NavLink's
built-in active matching (use exact where needed or the end prop for exact
matching) which automatically sets aria-current="page"; update any active
className logic to use NavLink's className callback (or style callback) to
return the correct classes based on isActive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tailwind.config.js (1)
10-12
: Nice addition: reusable glow token for navbar visuals.Value is valid for Tailwind’s drop-shadow filter. Tiny polish: prefer modern color syntax.
Apply:
- glow: "0 0 8px rgba(255, 255, 255, 0.6)", // white glow + glow: "0 0 8px rgb(255 255 255 / 0.6)", // white glow
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tailwind.config.js
(1 hunks)
🔇 Additional comments (2)
tailwind.config.js (2)
14-16
: LGTM.No plugins needed for this change. Compatible with darkMode: 'class'.
4-7
: Content globs are correct — no action required.
index.html is at the repo root; 'drop-shadow-glow' is used in src/components/Navbar.tsx (lines 62,64) and is defined in tailwind.config.js, so the JIT scanner will pick it up.
im done with it |
okay @janvis11 , thanks.. I will review tonight |
Related Issue
Enhance Navbar Styling with Better Headings and Hover Effects
Description
This PR enhances the Navbar UI/UX with improved styling and interactions:
Added better typography for headings
Improved hover effects for navbar links
Applied glassmorphism effect with blur and subtle gradient overlay
Polished overall navbar aesthetics for a modern look
How Has This Been Tested?
Verified responsiveness across desktop and mobile.
Keeps the existing code structure intact.
Enhancement inspired by modern UI trends.
Screenshots (if applicable)
Type of Change
Summary by CodeRabbit
New Features
Enhancements
Refactor