-
Notifications
You must be signed in to change notification settings - Fork 518
Fix Home navbar button to scroll to top on landing page #860
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
📝 WalkthroughWalkthroughThe changes implement in-page smooth scrolling for the landing page Home button. An anchor div with id "home" is added to the top of the hero section, and the Navbar Home link is modified to scroll to this anchor instead of navigating to the root path. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
landing-page/src/Pages/Landing page/Navbar.tsx (1)
194-196: Critical: Mobile menu Home link not updated with the same fix.The mobile menu still uses
to="/"without the scroll behavior, creating an inconsistent user experience. Mobile users won't get the scroll-to-top functionality that this PR aims to provide.🔎 Proposed fix for mobile menu consistency
- <NavLink to="/" onClick={() => setIsOpen(false)}> + <NavLink to="#home" isScrollLink={true} onClick={() => setIsOpen(false)}> Home </NavLink>
🧹 Nitpick comments (1)
landing-page/src/Pages/Landing page/Home1.tsx (1)
18-18: Consider improving anchor placement to avoid potential layout issues.The empty
<div id="home"></div>anchor works but could cause unintended spacing. Consider one of these alternatives:
- Add the
id="home"directly to the existing<section>element at line 17- Position the anchor absolutely to remove it from the document flow
🔎 Alternative approaches
Option 1: Add ID to the section element (preferred)
- <section className="w-full px-8 py-12 grid grid-cols-1 md:grid-cols-2 items-center gap-8 max-w-6xl mx-auto bg-white dark:bg-black transition-colors duration-300"> - <div id="home"></div> + <section id="home" className="w-full px-8 py-12 grid grid-cols-1 md:grid-cols-2 items-center gap-8 max-w-6xl mx-auto bg-white dark:bg-black transition-colors duration-300">Option 2: Use absolute positioning
- <div id="home"></div> + <div id="home" className="absolute top-0"></div>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
landing-page/src/Pages/Landing page/Home1.tsxlanding-page/src/Pages/Landing page/Navbar.tsx
🔇 Additional comments (1)
landing-page/src/Pages/Landing page/Navbar.tsx (1)
147-147: Desktop Home navigation correctly implements smooth scrolling.The change to use
to="#home"withisScrollLink={true}properly leverages the existing scroll mechanism and addresses the issue for desktop users.
What was fixed
The Home button in the landing page navbar did not scroll the page back to the top when clicked.
Why it happened
The Home link routed to
/, and when users were already on the landing page, React Router did not trigger navigation or reset the scroll position.How it was fixed
homeanchor at the top of the landing page.This keeps navigation consistent with other section links and avoids unnecessary routing logic.
Fixes #856
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.