enhance navbar#617
Open
Luffy-456 wants to merge 1 commit into
Open
Conversation
- Added animated sliding pill for desktop navigation hover states - Refactored nav links into a reusable navItems array - Added scroll detection for dynamic navbar glass effect - Applied backdrop blur and translucent background on scroll - Improved navbar visual polish in both light and dark modes - Fixed mobile theme toggle icon visibility - Added custom opening animation for mobile menu
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughNavbar component refactored with a centralized ChangesNavbar modernization with scroll tracking, sliding pill, and mobile menu improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/index.css (2)
94-104: ⚡ Quick winPrefer transform/opacity-only animation to avoid expensive blur repainting.
Animating
filter: blur()here can introduce visible jank on low-end/mobile devices. Keep the effect toopacity + transformfor smoother menu open.Proposed change
`@layer` utilities{ `@keyframes` fade-in{ from { opacity:0; transform: translateY(20px); - filter:blur(45px); } to{ opacity:1; transform: translateY(0); - filter:blur(0); } } .ani-fade-in{ animation: fade-in 0.8s ease-out both; } }Also applies to: 106-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.css` around lines 94 - 104, The keyframes animation "fade-in" (and the similar block at lines 106-108) currently animates filter: blur(), which is expensive; remove the filter property from both from/to blocks and only animate opacity and transform (translateY) so the menu opens smoothly on low-end devices — update the `@keyframes` fade-in and the other keyframe to drop filter: blur(…) entries and ensure only opacity and transform are animated.
93-109: ⚡ Quick winAdd reduced-motion fallback for the mobile menu animation.
There’s no
prefers-reduced-motionhandling, so motion-sensitive users still get the transition. Add an opt-out utility override.Proposed change
`@layer` utilities{ `@keyframes` fade-in{ @@ .ani-fade-in{ animation: fade-in 0.8s ease-out both; } + + `@media` (prefers-reduced-motion: reduce) { + .ani-fade-in { + animation: none; + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.css` around lines 93 - 109, Add a prefers-reduced-motion fallback so users who opt out of motion aren't animated: wrap an `@media` (prefers-reduced-motion: reduce) rule that targets the fade-in keyframes and the .ani-fade-in utility (the `@keyframes` fade-in and the .ani-fade-in class) and override the animation to none (and set static properties like opacity:1 and transform:none if needed) so the mobile menu appears instantly for reduced-motion users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Navbar.tsx`:
- Around line 22-29: The useEffect adds a scroll listener but never initializes
scrolled, so call the handler once on mount and add the listener as passive;
specifically, inside the useEffect where handleScroll(), setScrolled, and
window.addEventListener("scroll", handleScroll) are used, invoke handleScroll()
immediately after defining it and change the addEventListener call to use {
passive: true } so the navbar state is correct on initial render and the scroll
listener is passive.
---
Nitpick comments:
In `@src/index.css`:
- Around line 94-104: The keyframes animation "fade-in" (and the similar block
at lines 106-108) currently animates filter: blur(), which is expensive; remove
the filter property from both from/to blocks and only animate opacity and
transform (translateY) so the menu opens smoothly on low-end devices — update
the `@keyframes` fade-in and the other keyframe to drop filter: blur(…) entries
and ensure only opacity and transform are animated.
- Around line 93-109: Add a prefers-reduced-motion fallback so users who opt out
of motion aren't animated: wrap an `@media` (prefers-reduced-motion: reduce) rule
that targets the fade-in keyframes and the .ani-fade-in utility (the `@keyframes`
fade-in and the .ani-fade-in class) and override the animation to none (and set
static properties like opacity:1 and transform:none if needed) so the mobile
menu appears instantly for reduced-motion users.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e65b63c3-d933-4ca8-ab1a-6fd01978b39f
📒 Files selected for processing (2)
src/components/Navbar.tsxsrc/index.css
Comment on lines
+22
to
+29
| useEffect( () => { | ||
| const handleScroll = () => { | ||
| setScrolled(window.scrollY>20); | ||
| }; | ||
|
|
||
| window.addEventListener("scroll",handleScroll); | ||
| return () => window.removeEventListener("scroll",handleScroll); | ||
| },[]); |
Contributor
There was a problem hiding this comment.
Initialize scroll-derived state on mount.
scrolled is only set after the first scroll event, so loading a page at a non-zero scroll position renders the wrong navbar style initially. Call the handler once in the effect (and make the listener passive).
Suggested fix
useEffect( () => {
const handleScroll = () => {
setScrolled(window.scrollY>20);
};
- window.addEventListener("scroll",handleScroll);
- return () => window.removeEventListener("scroll",handleScroll);
+ handleScroll();
+ window.addEventListener("scroll", handleScroll, { passive: true });
+ return () => window.removeEventListener("scroll", handleScroll);
},[]);📝 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.
Suggested change
| useEffect( () => { | |
| const handleScroll = () => { | |
| setScrolled(window.scrollY>20); | |
| }; | |
| window.addEventListener("scroll",handleScroll); | |
| return () => window.removeEventListener("scroll",handleScroll); | |
| },[]); | |
| useEffect( () => { | |
| const handleScroll = () => { | |
| setScrolled(window.scrollY>20); | |
| }; | |
| handleScroll(); | |
| window.addEventListener("scroll", handleScroll, { passive: true }); | |
| return () => window.removeEventListener("scroll", handleScroll); | |
| },[]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Navbar.tsx` around lines 22 - 29, The useEffect adds a scroll
listener but never initializes scrolled, so call the handler once on mount and
add the listener as passive; specifically, inside the useEffect where
handleScroll(), setScrolled, and window.addEventListener("scroll", handleScroll)
are used, invoke handleScroll() immediately after defining it and change the
addEventListener call to use { passive: true } so the navbar state is correct on
initial render and the scroll listener is passive.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Description
The current navbar is functional but lacks a modern and visually appealing design. The overall appearance feels basic and doesn't fully align with UI/UX standards used in modern web applications.
Changes Made:
navItemsarrayHow Has This Been Tested?
Screenshots (if applicable)
2026-05-30.13-30-07.mp4
Type of Change