-
Notifications
You must be signed in to change notification settings - Fork 159
Properly hide Navbar when it's not visible #205
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
Properly hide Navbar when it's not visible #205
Conversation
|
@praveenzsp is attempting to deploy a commit to the AJEET PRATAP SINGH's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe navbar component's animation now includes Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
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)
apps/web/src/components/landing-sections/navbar.tsx (1)
15-15: Optional: Simplify boolean initialization.The ternary
isPricingPage ? true : falsecan be simplified to justisPricingPagesince it's already a boolean value.Apply this diff:
- const [showNavbar, setShowNavbar] = useState(isPricingPage ? true : false); + const [showNavbar, setShowNavbar] = useState(isPricingPage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/app/(main)/(landing)/layout.tsx(0 hunks)apps/web/src/components/landing-sections/navbar.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/src/app/(main)/(landing)/layout.tsx
🔇 Additional comments (3)
apps/web/src/components/landing-sections/navbar.tsx (3)
49-49: Thedisplay: 'none'addition correctly prevents interaction when hidden.Assuming the Navbar component is still being rendered somewhere in the application, adding
display: 'none'alongsideopacity: 0is the correct approach to fix the clickability issue. Framer Motion will applydisplay: 'none'after the opacity transition completes, ensuring the fade animation still works while preventing any interaction when hidden.
113-141: Verify mobile menu accessibility when navbar is hidden.When
showNavbaris false, the entire navbar including the mobile menu will havedisplay: 'none'applied. This means users on non-pricing pages who are scrolled to the top (wherescrollYProgress === 0) cannot access the mobile menu at all.Please verify this is the intended behavior, especially for mobile users who may need navigation access regardless of scroll position.
49-49: The review comment is based on an incorrect premise and should be dismissed.The verification confirms that the Navbar component is still actively being used in the codebase—specifically, it's imported and rendered in
apps/web/src/app/(main)/(landing)/page.tsx(lines 8 and 18). The AI summary claiming that the Navbar was "removed from layout rendering" is inaccurate.Since the Navbar is still being rendered, the change to add
display: 'none'on line 49 is necessary and appropriate. This enhancement ensures that whenshowNavbaris false (based on scroll position), the navbar becomes truly invisible and non-interactive, not just optically hidden. Combiningopacity: 0withdisplay: 'none'is a best practice for animation libraries like Framer Motion.Likely an incorrect or invalid review comment.
|
Hey @apsinghdev, this makes my first ever open source contribution, though small it feels good to get into OS through Opensox. Appreciate your work and efforts. |
|
@praveenzsp thanks for your contributions! im just occupied with some personal stuff and will review this very soon! 🙏 |
Sure, take your time |
|
@apsinghdev good to merge |
783158d to
2f5e536
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@praveenzsp nice work! 🔥 |
Fixes #152
This PR focus on properly hiding the navbar and nav links when it is not shown.
Earlier when the navbar is hidden, all its nav links are still clickable which is not supposed to happen.
With this PR, navbar is properly hidden and all nav links also disappear properly for better UX.
Before - Nav links still clickable
before.mp4
After - Navbar is hidden completely
after.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.