🎨 Enhanced Responsive Navbar with Animations, Cropping Fixes & Smooth Transitions.#69
Conversation
|
@Anas2604-web is attempting to deploy a commit to the idan lodzki's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Thanks for your contribution! Join our Slack: https://join.slack.com/t/opsimate/shared_invite/zt-39bq3x6et-NrVCZzH7xuBGIXmOjJM7gA Please make sure to include an image with your PR — it really helps us review and understand the changes better. Only in rare cases will we accept a PR without one. Also, take a moment to review your code to ensure it’s clear, readable, and easy to follow. PRs from contributors who haven’t joined our Slack community or starred the repository won’t be reviewed — we’d love to see you join and be part of the project! |
WalkthroughAdds a client-side Navbar with IntersectionObserver-based active-section tracking, scroll-triggered blur styling, mobile menu toggle and active-link classes, plus corresponding global CSS for nav visuals and scroll padding. Changes
Assessment against linked issues
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
🧹 Nitpick comments (2)
components/Navbar.tsx (2)
95-99: Consider more robust href parsing for active state.The current logic
item.href.replace("/#", "")works for the current navigation items but is fragile. It assumes all internal links follow the/#sectionpattern and will silently fail for other formats like/sectionor#section.Apply this diff for more robust matching:
- className={`nav-link text-surface-700 dark:text-surface-300 hover:text-primary-500 dark:hover:text-primary-400 font-medium ${ - activeSection === item.href.replace("/#", "") - ? "nav-link-active" - : "" - }`} + className={`nav-link text-surface-700 dark:text-surface-300 hover:text-primary-500 dark:hover:text-primary-400 font-medium ${ + activeSection === item.href.split("#").pop() + ? "nav-link-active" + : "" + }`}This handles
/#features,/page#features,#features, andhttps://example.com/#featuresconsistently by extracting everything after the last#. External links without hashes will safely not match any section.
154-158: Apply the same robust href parsing here.The mobile navigation uses the same fragile
item.href.replace("/#", "")logic as the desktop version.Apply this diff to match the improved desktop logic:
- className={`text-gray-700 hover:text-blue-600 font-medium transition-colors duration-200 px-2 py-1 ${ - activeSection === item.href.replace("/#", "") - ? "text-primary-600 dark:text-primary-400" - : "" - }`} + className={`text-gray-700 hover:text-blue-600 font-medium transition-colors duration-200 px-2 py-1 ${ + activeSection === item.href.split("#").pop() + ? "text-primary-600 dark:text-primary-400" + : "" + }`}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Navbar.tsx(8 hunks)
🔇 Additional comments (7)
components/Navbar.tsx (7)
1-3: LGTM! Proper client component setup.The "use client" directive and useEffect import are correctly configured for Next.js 14's app router with client-side interactivity.
22-23: LGTM! State variables properly initialized.Both
activeSectionandscrolledstates are appropriately typed and initialized for their intended use cases.
48-64: LGTM! Proper IntersectionObserver implementation.The active section tracking is correctly implemented with appropriate rootMargin to highlight sections when they're centered in the viewport, and cleanup is properly handled.
66-72: LGTM! Scroll state properly initialized.The scroll blur effect is correctly implemented, and the immediate invocation of
handleScroll()on line 69 ensures the navbar has the correct style on mount, even for deep-linked pages. This addresses the previous review concern.
75-81: LGTM! Conditional styling properly applied.The scroll-based navbar styling toggles correctly between states, providing the intended visual feedback.
124-131: LGTM! Contact CTA properly implemented.The Contact Us button is correctly implemented as a prominent call-to-action with appropriate mailto link handling.
169-176: LGTM! Mobile contact link properly implemented.The mobile Contact Us link is correctly styled and includes the menu close handler for good UX.
| // constants/contact.ts | ||
| export const CONTACT_EMAIL = 'idan.lut@gmail.com'; | ||
| export const CONTACT_EMAIL = "idan.lut@gmail.com"; | ||
| export const CONTACT_MAILTO = `mailto:${CONTACT_EMAIL}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Import the constant instead of duplicating it.
The comment indicates this constant lives in constants/contact.ts, but it's defined inline here. This creates duplication and maintenance burden.
Apply this diff to import the constant:
+import { CONTACT_EMAIL, CONTACT_MAILTO } from "@/constants/contact";
+
-// constants/contact.ts
-export const CONTACT_EMAIL = "idan.lut@gmail.com";
-export const CONTACT_MAILTO = `mailto:${CONTACT_EMAIL}`;Then update the contactLink definition:
const contactLink: NavigationItem = {
name: "Contact Us",
href: CONTACT_MAILTO,
external: true,
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/Navbar.tsx around lines 10 to 12, the mailto constant is
duplicated locally; remove the inline CONTACT_EMAIL/CONTACT_MAILTO definitions
and instead import CONTACT_MAILTO from constants/contact.ts, then update the
contactLink definition to use the imported CONTACT_MAILTO; ensure the import is
added at the top of the file and unused local constants are deleted.
|
Hi @idanlodzki 👋 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/Navbar.tsx (2)
139-143: Use theme-aware color classes for consistency.The mobile menu button uses hardcoded colors (
text-gray-700 hover:text-blue-600) instead of the theme-aware classes used throughout the rest of the component.Apply this diff to maintain theme consistency:
<button onClick={() => setIsMenuOpen(!isMenuOpen)} - className="text-gray-700 hover:text-blue-600 transition-colors duration-200" + className="text-surface-700 dark:text-surface-300 hover:text-primary-500 dark:hover:text-primary-400 transition-colors duration-200" >
148-148: Use theme-aware border color for consistency.The mobile navigation container uses a hardcoded border color (
border-gray-100) that doesn't adapt to dark mode, unlike the main navbar which usesborder-surface-200 dark:border-surface-800.Apply this diff:
- <div className="md:hidden py-4 border-t border-gray-100"> + <div className="md:hidden py-4 border-t border-surface-200 dark:border-surface-800">
♻️ Duplicate comments (1)
components/Navbar.tsx (1)
10-12: Remove duplicate constants and import from the correct location.These constants are duplicated from
constants/contact.ts. Additionally, the quote style here (single quotes on line 11) is inconsistent with the double quotes used inconstants/contact.tsper the AI summary.Apply this diff to import the constants instead:
+import { CONTACT_EMAIL, CONTACT_MAILTO } from "@/constants/contact"; + -// constants/contact.ts -export const CONTACT_EMAIL = 'idan.lut@gmail.com'; -export const CONTACT_MAILTO = `mailto:${CONTACT_EMAIL}`;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Navbar.tsx(8 hunks)
🔇 Additional comments (8)
components/Navbar.tsx (8)
1-1: LGTM!The "use client" directive is correctly placed and necessary for the client-side interactivity (hooks and DOM APIs) introduced in this component.
3-3: LGTM!The expanded React imports are necessary for the state management and side effects introduced in this component.
22-23: LGTM!The state variables are properly typed and initialized, supporting the active section tracking and scroll-based blur effect.
48-64: LGTM!The IntersectionObserver correctly tracks visible sections and updates the active state. The cleanup properly unobserves all sections on unmount.
66-72: LGTM!The scroll handler correctly initializes state on mount (addressing the previous review feedback) and properly manages the event listener lifecycle.
75-81: LGTM!The dynamic navbar styling correctly applies theme-aware classes and transitions based on scroll state.
124-131: LGTM!The Contact Us button is properly implemented with appropriate primary button styling and correct mailto link.
169-176: LGTM!The mobile Contact Us link is properly implemented with theme-aware styling and correct menu-closing behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/Navbar.tsx (2)
10-12: Import the constant instead of duplicating it.The inline constant definitions create duplication, as indicated by the comment. Import
CONTACT_EMAILandCONTACT_MAILTOfromconstants/contact.tsinstead.
154-154: Use theme-aware color classes for consistency.Line 154 still uses hardcoded colors (
text-gray-700 hover:text-blue-600) instead of theme-aware classes, which is inconsistent with the desktop navigation and doesn't properly support dark mode.Apply this diff:
- className={`text-gray-700 hover:text-blue-600 font-medium transition-colors duration-200 px-2 py-1 ${ + className={`text-surface-700 dark:text-surface-300 hover:text-primary-500 dark:hover:text-primary-400 font-medium transition-colors duration-200 px-2 py-1 ${ !item.external && activeSection === item.href.split("#").pop() ? "text-primary-600 dark:text-primary-400" : "" }`}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Navbar.tsx(8 hunks)
🔇 Additional comments (1)
components/Navbar.tsx (1)
48-72: Well-implemented active section tracking and scroll effects.The IntersectionObserver with centered rootMargin provides smooth active section detection, and initializing the scroll handler on mount (line 69) correctly addresses the previous deep-link issue. Both effects include proper cleanup.
| <button | ||
| onClick={() => setIsMenuOpen(!isMenuOpen)} | ||
| className="text-gray-700 hover:text-blue-600 transition-colors duration-200" | ||
| className="text-surface-700 dark:text-surface-300 hover:text-primary-500 dark:hover:text-primary-400 transition-colors duration-200" | ||
| > | ||
| {isMenuOpen ? <X size={24} /> : <Menu size={24} />} | ||
| </button> |
There was a problem hiding this comment.
Add accessibility attributes to the mobile menu button.
The mobile menu toggle button lacks aria-label and aria-expanded attributes, which are essential for screen reader users to understand its purpose and state.
Apply this diff to add the missing attributes:
<button
onClick={() => setIsMenuOpen(!isMenuOpen)}
className="text-surface-700 dark:text-surface-300 hover:text-primary-500 dark:hover:text-primary-400 transition-colors duration-200"
+ aria-label="Toggle navigation menu"
+ aria-expanded={isMenuOpen}
>
{isMenuOpen ? <X size={24} /> : <Menu size={24} />}
</button>📝 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.
| <button | |
| onClick={() => setIsMenuOpen(!isMenuOpen)} | |
| className="text-gray-700 hover:text-blue-600 transition-colors duration-200" | |
| className="text-surface-700 dark:text-surface-300 hover:text-primary-500 dark:hover:text-primary-400 transition-colors duration-200" | |
| > | |
| {isMenuOpen ? <X size={24} /> : <Menu size={24} />} | |
| </button> | |
| <button | |
| onClick={() => setIsMenuOpen(!isMenuOpen)} | |
| className="text-surface-700 dark:text-surface-300 hover:text-primary-500 dark:hover:text-primary-400 transition-colors duration-200" | |
| aria-label="Toggle navigation menu" | |
| aria-expanded={isMenuOpen} | |
| > | |
| {isMenuOpen ? <X size={24} /> : <Menu size={24} />} | |
| </button> |
🤖 Prompt for AI Agents
In components/Navbar.tsx around lines 137 to 142, the mobile menu toggle button
is missing accessibility attributes; update the button to include an appropriate
aria-label (e.g., "Open menu" / "Close menu" or a neutral "Toggle menu") and add
aria-expanded set to the boolean isMenuOpen so screen readers know its state;
optionally include aria-controls pointing to the mobile menu element's id if
present to associate the button with the menu.
2 participants
Closes #68
This PR refines the existing Navbar by introducing smoother interactions and visual improvements for a more polished user experience.
🔧 Key Enhancements
✨ Smooth scrolling & subtle animations for better navigation flow
🧩 Fixed cropping issue where section headers were partially hidden on scroll
📱 Improved responsiveness across mobile and desktop views
🎨 Enhanced hover effects and consistent color transitions
A visually refined, accessible, and user-friendly Navbar that elevates the overall experience and aligns with the project’s modern design standards.
💬 Note: Hope you like the updates! ✨
I’ve focused on enhancing both functionality and UX to make navigation feel smoother and more modern.
Summary by CodeRabbit
New Features
Style