feat: add back to top scroll button#103
Conversation
|
@zaibamachhaliya is attempting to deploy a commit to the niharika-mente's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughA new ChangesBackToTop Button Feature
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
|
Hi @maintainer, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/BackToTop.jsx (1)
14-18: ⚡ Quick winUse a passive scroll listener.
Marking the scroll listener as passive helps avoid main-thread scroll blocking.
Suggested fix
- window.addEventListener("scroll", handleScroll); + window.addEventListener("scroll", handleScroll, { passive: true }); return () => { - window.removeEventListener("scroll", handleScroll); + 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 `@components/BackToTop.jsx` around lines 14 - 18, The scroll event listener in the event setup is not marked as passive, which can cause main-thread blocking during scroll. Modify both the addEventListener and removeEventListener calls to include a third argument that specifies the listener options with passive set to true. This applies to the window.addEventListener call for the scroll event with handleScroll handler and its corresponding removeEventListener in the cleanup function.
🤖 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 `@components/BackToTop.jsx`:
- Around line 9-19: In the useEffect hook of the BackToTop component, after
registering the scroll event listener with window.addEventListener, invoke
handleScroll() once to initialize the button visibility based on the current
scroll position. This ensures that if the page loads with a scroll position
already greater than 200px (such as after scroll position restoration), the
button will display correctly instead of remaining hidden until the next scroll
event occurs.
---
Nitpick comments:
In `@components/BackToTop.jsx`:
- Around line 14-18: The scroll event listener in the event setup is not marked
as passive, which can cause main-thread blocking during scroll. Modify both the
addEventListener and removeEventListener calls to include a third argument that
specifies the listener options with passive set to true. This applies to the
window.addEventListener call for the scroll event with handleScroll handler and
its corresponding removeEventListener in the cleanup function.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a522fbfb-9ba6-4dcb-a0cc-a11bdde7b718
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (3)
app/layout.tsxcomponents/BackToTop.jsxpackage.json
|
@zaibamachhaliya, try to give images/mockups/videos for any UI change 🙂 |
TarunyaProgrammer
left a comment
There was a problem hiding this comment.
Thank you for the contribution. The feature itself is useful and the implementation is straightforward, but a few changes are needed before this can be merged:
- Please revert the
mongooseversion upgrade and the relatedpackage-lock.jsonchanges. These modifications are unrelated to the Back to Top button and introduce unnecessary dependency noise into the PR. - The project uses TypeScript. Please rename
components/BackToTop.jsxtocomponents/BackToTop.tsx. - The component currently uses hardcoded cyan color values. Please use the existing design system tokens (
bg-primary,border-primary, etc.) to keep styling consistent with the rest of the application. - Consider calling
handleScroll()once inside theuseEffectafter registering the scroll listener. This ensures the button is shown correctly when the page loads with a restored scroll position.
Once these items are addressed, the feature should be ready for another review.
|
All requested changes have been implemented! ✅ Changes made:
Could you please review the updated changes? |
TarunyaProgrammer
left a comment
There was a problem hiding this comment.
The button currently uses bg-primary text-primary-foreground, but --primary is a very light cyan (#59deca) and --primary-foreground resolves to near-white. This results in very low contrast (~1.6:1), which does not meet WCAG accessibility requirements and makes the icon difficult to see.
Consider using a darker foreground color instead, consistent with other primary buttons in the project:
- bg-primary text-primary-foreground
+ bg-primary text-blackPlease update the button styling to ensure sufficient contrast before merging.
|
@TarunyaProgrammer |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@components/BackToTop.tsx`:
- Line 39: In the BackToTop component, replace the hardcoded text-black class
with text-primary-foreground in the className containing bg-primary. This
ensures the text color uses the design token system for proper theme consistency
and contrast instead of being hardcoded to black.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87c6eb37-4803-4b5e-b35b-894277fc1af7
📒 Files selected for processing (1)
components/BackToTop.tsx
Summary
Implemented a floating "Back to Top" button to improve navigation and user experience on long pages.
Changes Made
Screenshots
Issue Fixed
Closes #98
Testing
Summary by CodeRabbit