-
Notifications
You must be signed in to change notification settings - Fork 108
feat: add scroll to top button with progress indicator #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
base: main
Are you sure you want to change the base?
Conversation
- Created ScrollToTop component with smooth animations - Added circular progress ring showing scroll position - Gradient purple design with hover effects and tooltip - Optimized scroll event handling with requestAnimationFrame - WCAG compliant with proper ARIA attributes - Responsive and mobile-friendly Closes AOSSIE-Org#204
WalkthroughA ScrollToTop React component was added and mounted in App.tsx inside the AuthProvider/Router tree. It shows a floating button after 300px scroll, renders a circular progress ring reflecting page scroll percentage, and smooth-scrolls to top when clicked. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User (browser)
participant C as ScrollToTop Component
participant W as Window (scroll/resize)
Note over U,C: Page loads
U->>C: mount()
C->>W: addEventListener(scroll)
C->>W: addEventListener(resize)
Note right of W: On scroll (debounced via requestAnimationFrame)
W-->>C: scroll event
C->>C: compute isVisible & scrollProgress
C-->>U: update UI (show/hide, ring offset)
alt User clicks button
U->>C: click()
C->>W: window.scrollTo({ top:0, behavior:"smooth" })
Note over W,C: smooth scroll animates back to top
end
Note over C: on unmount -> remove listeners, cancel RAF
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 2
🧹 Nitpick comments (2)
Frontend/src/components/ui/scroll-to-top.tsx (2)
49-51: Consider CSS custom property for dynamic shadow.The inline
styleprop with a template literal recalculates on every render. Moving this to a CSS custom property can improve performance slightly by reducing JavaScript string concatenation overhead.Apply this diff to use a CSS custom property:
onClick={scrollToTop} - className="group relative p-4 rounded-full bg-gradient-to-br from-purple-600 via-purple-700 to-indigo-600 text-white shadow-2xl hover:shadow-purple-500/50 transition-all duration-300 hover:scale-110 focus:outline-none focus:ring-4 focus:ring-purple-500/50 active:scale-95 backdrop-blur-sm" + className="group relative p-4 rounded-full bg-gradient-to-br from-purple-600 via-purple-700 to-indigo-600 text-white shadow-2xl hover:shadow-purple-500/50 transition-all duration-300 hover:scale-110 focus:outline-none focus:ring-4 focus:ring-purple-500/50 active:scale-95 backdrop-blur-sm [box-shadow:0_10px_40px_rgba(147,51,234,var(--shadow-alpha))]" aria-label="Scroll to top" style={{ - boxShadow: `0 10px 40px rgba(147, 51, 234, ${scrollProgress / 200})`, + '--shadow-alpha': scrollProgress / 200, + } as React.CSSProperties} - }} >
45-91: Link tooltip to button for screen readers.The tooltip (Lines 88–91) is purely visual and not announced to screen readers. Adding
aria-describedbyon the button would properly associate the tooltip text with the control, improving accessibility for users relying on assistive technology.Apply this diff to link the tooltip:
<button onClick={scrollToTop} className="group relative p-4 rounded-full bg-gradient-to-br from-purple-600 via-purple-700 to-indigo-600 text-white shadow-2xl hover:shadow-purple-500/50 transition-all duration-300 hover:scale-110 focus:outline-none focus:ring-4 focus:ring-purple-500/50 active:scale-95 backdrop-blur-sm" aria-label="Scroll to top" + aria-describedby="scroll-to-top-tooltip" style={{ boxShadow: `0 10px 40px rgba(147, 51, 234, ${scrollProgress / 200})`, }} > {/* ... button content ... */} </button> {/* Tooltip */} - <div className="absolute bottom-full right-0 mb-2 px-3 py-1.5 bg-gray-900 text-white text-sm rounded-lg opacity-0 group-hover:opacity-100 transition-opacity duration-200 whitespace-nowrap pointer-events-none"> + <div + id="scroll-to-top-tooltip" + role="tooltip" + className="absolute bottom-full right-0 mb-2 px-3 py-1.5 bg-gray-900 text-white text-sm rounded-lg opacity-0 group-hover:opacity-100 transition-opacity duration-200 whitespace-nowrap pointer-events-none" + > Back to top <div className="absolute top-full right-4 -mt-1 border-4 border-transparent border-t-gray-900" /> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Frontend/src/App.tsx(2 hunks)Frontend/src/components/ui/scroll-to-top.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Frontend/src/App.tsx (1)
Frontend/src/components/ui/scroll-to-top.tsx (1)
ScrollToTop(4-94)
🔇 Additional comments (2)
Frontend/src/App.tsx (1)
23-23: LGTM: Clean integration of ScrollToTop component.The import and placement are correct. Rendering the component inside the Router context but outside individual Routes ensures it's available globally across all pages while maintaining proper React context access.
Also applies to: 49-49
Frontend/src/components/ui/scroll-to-top.tsx (1)
32-37: LGTM: Clean scroll-to-top implementation.The function correctly uses
window.scrollTowithbehavior: "smooth"for a smooth scrolling experience. The implementation is straightforward and appropriate for this use case.
- Add requestAnimationFrame throttling for scroll events - Fix division by zero on non-scrollable pages - Calculate SVG radius dynamically to match rendered size - Add proper cleanup and accessibility attributes
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: 1
♻️ Duplicate comments (1)
Frontend/src/components/ui/scroll-to-top.tsx (1)
44-59: Percentage-based radius still mismatches pixel-based circumference calculation.Although
circleRadiusis now computed dynamically (Lines 44-59), the SVG circles at Lines 96-107 still user="45%", which references the SVG's coordinate system, while the circumference calculation at Lines 68-69 uses a pixel value derived fromgetBoundingClientRect. If the SVG has non-square dimensions, padding, or any coordinate-system discrepancies, the progress ring will not align correctly with the actual circle radius.The previous review recommended using a fixed-pixel radius consistently or defining a fixed
viewBoxto eliminate this mismatch.Apply this diff to use a fixed pixel radius consistently:
<svg ref={svgRef} className="absolute inset-0 w-full h-full -rotate-90" + viewBox="0 0 56 56" style={{ filter: 'drop-shadow(0 0 8px rgba(168, 85, 247, 0.4))' }} > <circle - cx="50%" - cy="50%" - r="45%" + cx="28" + cy="28" + r="24" fill="none" stroke="rgba(255, 255, 255, 0.2)" strokeWidth="2" /> <circle - cx="50%" - cy="50%" - r="45%" + cx="28" + cy="28" + r="24" fill="none" stroke="white" strokeWidth="2" strokeDasharray={circumference} strokeDashoffset={offset} strokeLinecap="round" className="transition-all duration-300" /> </svg>Then update the radius calculation to use the fixed value:
useEffect(() => { const updateCircleRadius = () => { - if (svgRef.current) { - const svgRect = svgRef.current.getBoundingClientRect(); - const computedRadius = (Math.min(svgRect.width, svgRect.height) / 2) * 0.45; - setCircleRadius(Math.max(computedRadius, 1)); - } + setCircleRadius(24); }; updateCircleRadius(); - window.addEventListener("resize", updateCircleRadius, { passive: true }); return () => { - window.removeEventListener("resize", updateCircleRadius); }; }, []);Also applies to: 96-115
🧹 Nitpick comments (2)
Frontend/src/components/ui/scroll-to-top.tsx (2)
68-69: Consider memoizing circumference calculations.The
circumferenceandoffsetcalculations run on every render. While the arithmetic is inexpensive, wrapping them inuseMemowould follow React best practices and avoid redundant computation.+ const circumference = useMemo(() => 2 * Math.PI * circleRadius, [circleRadius]); + const offset = useMemo(() => circumference * (1 - scrollProgress / 100), [circumference, scrollProgress]); - const circumference = 2 * Math.PI * circleRadius; - const offset = circumference * (1 - scrollProgress / 100);
44-59: Consider throttling the resize listener.The scroll handler correctly uses
requestAnimationFramethrottling, but the resize listener does not. Resize events can fire frequently during window resizing, causing multiple DOM reads and state updates. Applying the same RAF throttling pattern would improve consistency and performance.useEffect(() => { + let resizeRafId: number | null = null; + const updateCircleRadius = () => { if (svgRef.current) { const svgRect = svgRef.current.getBoundingClientRect(); const computedRadius = (Math.min(svgRect.width, svgRect.height) / 2) * 0.45; setCircleRadius(Math.max(computedRadius, 1)); } + resizeRafId = null; }; + const handleResize = () => { + if (resizeRafId === null) { + resizeRafId = requestAnimationFrame(updateCircleRadius); + } + }; + updateCircleRadius(); - window.addEventListener("resize", updateCircleRadius, { passive: true }); + window.addEventListener("resize", handleResize, { passive: true }); return () => { - window.removeEventListener("resize", updateCircleRadius); + window.removeEventListener("resize", handleResize); + if (resizeRafId !== null) { + cancelAnimationFrame(resizeRafId); + } }; }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Frontend/src/components/ui/scroll-to-top.tsx(1 hunks)
🔇 Additional comments (1)
Frontend/src/components/ui/scroll-to-top.tsx (1)
11-42: RAF throttling and zero-division guard properly implemented.The scroll handler now correctly throttles updates with
requestAnimationFrameand guards against division by zero when the page is non-scrollable. This addresses the performance andNaNissues flagged in previous reviews.
| <div | ||
| className={`fixed bottom-8 right-8 z-50 transition-all duration-500 ease-out ${ | ||
| isVisible ? "opacity-100 translate-y-0" : "opacity-0 translate-y-16 pointer-events-none" | ||
| }`} | ||
| role="complementary" | ||
| aria-label="Scroll to top navigation" | ||
| > |
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.
Remove semantically incorrect ARIA role from wrapper.
The outer <div> uses role="complementary", which is intended for landmark regions like sidebars that complement the main content. A scroll-to-top button wrapper is not a complementary landmark. The inner <button> already has the correct aria-label="Scroll to top" and type="button", making the outer role redundant and potentially confusing for screen readers.
Apply this diff to remove the incorrect role:
<div
className={`fixed bottom-8 right-8 z-50 transition-all duration-500 ease-out ${
isVisible ? "opacity-100 translate-y-0" : "opacity-0 translate-y-16 pointer-events-none"
}`}
- role="complementary"
- aria-label="Scroll to top navigation"
>📝 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.
| <div | |
| className={`fixed bottom-8 right-8 z-50 transition-all duration-500 ease-out ${ | |
| isVisible ? "opacity-100 translate-y-0" : "opacity-0 translate-y-16 pointer-events-none" | |
| }`} | |
| role="complementary" | |
| aria-label="Scroll to top navigation" | |
| > | |
| <div | |
| className={`fixed bottom-8 right-8 z-50 transition-all duration-500 ease-out ${ | |
| isVisible ? "opacity-100 translate-y-0" : "opacity-0 translate-y-16 pointer-events-none" | |
| }`} | |
| > |
🤖 Prompt for AI Agents
In Frontend/src/components/ui/scroll-to-top.tsx around lines 72 to 78, the outer
div incorrectly uses role="complementary" which is a landmark role and not
appropriate for a scroll-to-top wrapper; remove the role attribute from the
outer div so the inner button remains the accessible control (it already has
aria-label and type), leaving the wrapper as a purely presentational element.
|
Raised a PR here here #205 .Please review and let me know if any further changes required. |
Closes #204
🚀 Added Scroll to Top Button
📝 Description
Implemented an enhanced scroll-to-top button with smooth animations and visual progress indicator to improve user navigation experience.
🎯 Changes Made
ScrollToTopcomponent atFrontend/src/components/ui/scroll-to-top.tsxApp.tsxfeature/scroll-to-top✨ Features
🛠 Technical Implementation
ArrowUp)requestAnimationFramethrottling for scroll events🧪 Testing
📸 Preview
Screen.Recording.2025-11-22.at.4.55.44.PM.1.mov
✅ Checklist
📦 Files Changed
Frontend/src/components/ui/scroll-to-top.tsx(new)Frontend/src/App.tsx(modified)Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.