-
Notifications
You must be signed in to change notification settings - Fork 457
Fix: Home button now scrolls to top smoothly on landing page (#856) #859
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?
Fix: Home button now scrolls to top smoothly on landing page (#856) #859
Conversation
📝 WalkthroughWalkthroughThese changes fix the Home button navigation on the landing page by adding a wrapper div with id="home" to the root component and implementing custom smooth-scroll behavior with a 90px offset to account for the sticky navbar height. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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)
117-137: Wrap logo links with NavLink component to enable smooth scroll behavior.The logo links (image and text, lines 117-137) currently use plain
<Link to="/"/>instead of theNavLinkwrapper. When users are already on the landing page and click the logo, no scroll action occurs.To match the Home button pattern and provide consistent UX, update both logo links to use:
<NavLink to="#home" isScrollLink={true}> {/* logo content */} </NavLink>The
#homeanchor is properly defined in App.tsx and the NavLink component already supports this scroll behavior.
🧹 Nitpick comments (1)
landing-page/src/Pages/Landing page/Navbar.tsx (1)
30-38: Scroll implementation correctly accounts for sticky navbar.The custom scroll calculation properly offsets by 90px to prevent content from hiding behind the fixed navbar.
Consider extracting the offset to a named constant for better maintainability:
Optional refactor: Extract magic number to constant
+ // Offset to account for sticky navbar height + const NAVBAR_OFFSET = 90; + const handleClick = (e: React.MouseEvent) => { if (isScrollLink) { e.preventDefault(); // Remove the # from the target const targetId = to.replace("#", ""); const element = document.getElementById(targetId); if (element) { // Scroll with offset to account for sticky navbar - const offset = 90; + const offset = NAVBAR_OFFSET; const elementTop = element.getBoundingClientRect().top + window.scrollY; const targetScroll = elementTop - offset; window.scrollTo({ top: targetScroll, behavior: "smooth", }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
landing-page/src/App.tsxlanding-page/src/Pages/Landing page/Navbar.tsx
🔇 Additional comments (3)
landing-page/src/Pages/Landing page/Navbar.tsx (2)
151-151: Desktop Home link updated consistently.The change to
#homewithisScrollLink={true}aligns with the pattern used for other section links (Features, About) and properly fixes the scroll behavior for the single-page layout.Note: This approach changes the URL to
/#homewhen clicked. If this is undesirable, or if future multi-page navigation is planned, consider implementing a solution that checks the current route and conditionally applies scroll behavior only when already on/, while preserving standard navigation from other routes.
198-200: Mobile Home link properly updated.The change maintains the menu-closing behavior while adding the scroll functionality, consistent with other mobile navigation items.
landing-page/src/App.tsx (1)
18-27: The wrapper div is safe and introduces no layout issues.The
<div id="home">is nested inside the Route element hierarchy, not a direct child of the.relative.pt-20container, so it has no impact on layout. App.css contains only minimal styling with no direct child selectors targeting container elements. The change correctly enables hash-based navigation viagetElementById()calls (as used in Navbar.tsx).
Summary
This PR fixes the issue where clicking Home on the landing page
did nothing when the user was already on
/.React Router does not trigger navigation if you are already on the
same route, so the scroll position was not reset.
Changes
id="home"at the top of the landing layout#homewithisScrollLinkWhy this works
The navbar already implements smooth scrolling for section anchors.
By using
#home, we reuse that logic and avoid router no-ops.Manual Testing
Fixes #856
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.