-
Notifications
You must be signed in to change notification settings - Fork 522
feat(ui): add floating back-to-top button for improved navigation #989
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 PR adds a new ScrollToTopButton component to the Home page and enhances MediaInfoPanel with exit animations and improved Location visibility handling. The scroll-to-top button appears after scrolling past a configurable threshold and smoothly returns users to the top of the page. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In @frontend/src/components/Media/MediaInfoPanel.tsx:
- Around line 29-40: The panel fails to animate on external closes because
useEffect only sets isVisible to true; change the effect to mirror the prop by
doing setIsVisible(show) so that when show becomes false the exit animation
runs; keep the existing handleClose behavior (which sets isVisible(false) and
calls onClose after 300ms) to preserve delayed internal closes and avoid calling
onClose from the effect so you don’t double-invoke it.
In @frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx:
- Around line 14-28: The effect that attaches the scroll listener (useEffect)
only updates visibility on scroll; call the same handler once on mount (and
whenever scrollContainerRef or threshold changes) to set initial visibility
immediately. Locate the useEffect, the scroller/handleScroll function, and the
setVisible call, and invoke handleScroll() after registering the event listener
(or before returning the cleanup) so the component checks the current scrollTop
against threshold on mount and when dependencies change.
- Around line 46-52: The Tailwind classes in ScrollToTopButton's className use
invalid spacing keys (top-35, right-15); update the positioning to use valid
scale values or arbitrary value syntax: replace top-35/right-15 in the className
string of the ScrollToTopButton component with either a nearest valid scale like
top-36/right-16 or explicit arbitrary values like top-[35px] and right-[15px] so
Tailwind v4 compiles correctly.
🧹 Nitpick comments (1)
frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx (1)
54-54: Consider using an icon component for better accessibility.The Unicode arrow character
↑works but may not render consistently across browsers and screen readers. Consider using an icon fromlucide-react(already used in the project) like<ChevronUp />or<ArrowUp />for better consistency and accessibility.♻️ Proposed refactor using lucide-react icon
-import { useEffect, useState } from 'react'; +import { useEffect, useState } from 'react'; +import { ChevronUp } from 'lucide-react';" > - ↑ + <ChevronUp className="h-5 w-5" /> </button>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
frontend/src/components/Media/MediaInfoPanel.tsxfrontend/src/components/ScrollToTopButton/ScrollToTopButton.tsxfrontend/src/pages/Home/Home.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/Home/Home.tsx (1)
frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx (1)
ScrollToTopButton(8-57)
🔇 Additional comments (5)
frontend/src/pages/Home/Home.tsx (2)
16-16: LGTM! ScrollToTopButton integration is correct.The component is properly imported and integrated with the existing
scrollableRef, which is correctly attached to the scrollable container on line 58. The button will track scroll position within the gallery's scrollable area.Also applies to: 83-83
52-52: Minor formatting improvement.Removed trailing whitespace from the title string.
frontend/src/components/Media/MediaInfoPanel.tsx (2)
78-86: Animation classes look good.The exit animation uses opacity and horizontal translation with a 300ms duration that matches the
setTimeoutdelay inhandleClose. The transform utilities follow Tailwind v4 syntax correctly.
131-144: This review comment cannot be substantiated.The code at lines 131-132 is internally consistent with the rest of the codebase. Both the conditional render and the
handleLocationClickfunction require bothlatitudeANDlongitudeto be present, which is logically sound since generating a valid Google Maps URL (https://maps.google.com/?q=${latitude},${longitude}) requires both coordinates.The referenced AI summary claiming the condition was "relaxed" could not be found in the repository, recent git history, or documentation. Without access to the actual summary being criticized, no inconsistency can be verified. The code itself contains no contradictions.
Likely an incorrect or invalid review comment.
frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx (1)
14-28: The scroll listener dependency array is correct for this implementation.The div element with
ref={scrollableRef}is always rendered and never replaced or conditionally unmounted. Only its children change based on theimages.lengthcondition. Since the container element remains in the DOM throughout the component's lifecycle, the current dependency array[scrollContainerRef, threshold]properly handles listener setup and cleanup.Likely an incorrect or invalid review comment.
| useEffect(() => { | ||
| const scroller = scrollContainerRef?.current || window; | ||
|
|
||
| const handleScroll = () => { | ||
| const scrollTop = | ||
| scroller instanceof Window | ||
| ? window.scrollY | ||
| : scroller.scrollTop; | ||
|
|
||
| setVisible(scrollTop > threshold); | ||
| }; | ||
|
|
||
| scroller.addEventListener('scroll', handleScroll); | ||
| return () => scroller.removeEventListener('scroll', handleScroll); | ||
| }, [scrollContainerRef, threshold]); |
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.
Check initial scroll position on mount.
The button visibility is only updated on scroll events. If the page is already scrolled past the threshold when the component mounts (e.g., user navigates back via browser history), the button won't appear until the user scrolls again.
🔧 Proposed fix to check initial scroll position
useEffect(() => {
const scroller = scrollContainerRef?.current || window;
const handleScroll = () => {
const scrollTop =
scroller instanceof Window
? window.scrollY
: scroller.scrollTop;
setVisible(scrollTop > threshold);
};
+ // Check initial scroll position
+ handleScroll();
+
scroller.addEventListener('scroll', handleScroll);
return () => scroller.removeEventListener('scroll', handleScroll);
}, [scrollContainerRef, threshold]);🤖 Prompt for AI Agents
In @frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx around lines
14 - 28, The effect that attaches the scroll listener (useEffect) only updates
visibility on scroll; call the same handler once on mount (and whenever
scrollContainerRef or threshold changes) to set initial visibility immediately.
Locate the useEffect, the scroller/handleScroll function, and the setVisible
call, and invoke handleScroll() after registering the event listener (or before
returning the cleanup) so the component checks the current scrollTop against
threshold on mount and when dependencies change.
| className=" | ||
| fixed top-35 right-15 z-50 | ||
| rounded-full bg-primary p-3 | ||
| text-white shadow-lg | ||
| transition-all duration-300 | ||
| hover:scale-110 hover:shadow-xl | ||
| " |
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.
🧩 Analysis chain
🌐 Web query:
Tailwind CSS v4 default spacing scale and position utilities top right
💡 Result:
-
Default spacing unit: 1 = 0.25rem (4px). The v4 spacing scale is derived from this and includes common values (name → rem → px): 0 → 0px; px → 1px; 0.5 → 0.125rem (2px); 1 → 0.25rem (4px); 1.5 → 0.375rem (6px); 2 → 0.5rem (8px); 2.5 → 0.625rem (10px); 3 → 0.75rem (12px); 3.5 → 0.875rem (14px); 4 → 1rem (16px); 5 → 1.25rem (20px); 6 → 1.5rem (24px); 7 → 1.75rem (28px); 8 → 2rem (32px); 9 → 2.25rem (36px); 10 → 2.5rem (40px); 11 → 2.75rem (44px); 12 → 3rem (48px); 14 → 3.5rem (56px); 16 → 4rem (64px); 20 → 5rem (80px); 24 → 6rem (96px); 28 → 7rem (112px); 32 → 8rem (128px); 36 → 9rem (144px); 40 → 10rem (160px); 44 → 11rem (176px); 48 → 12rem (192px); 52 → 13rem (208px); 56 → 14rem (224px); 60 → 15rem (240px); 64 → 16rem (256px); 72 → 18rem (288px); 80 → 20rem (320px); 96 → 24rem (384px). [1]
-
Position utilities (top/right/etc.): v4 provides top-, right-, inset-, inset-x/inset-y, negative variants, and arbitrary/custom-value forms (e.g. top-0, right-0 to pin to the corner; inset-[3px], inset-(--var) for custom/CSS-variable). Example to pin an absolutely positioned element to the top-right corner: class="absolute top-0 right-0". [2]
Sources:
[1] Tailwind docs — Customizing spacing / default spacing scale.
[2] Tailwind docs — top / right / bottom / left (position/inset utilities).
🏁 Script executed:
# Find Tailwind configuration in the repository
fd -e "tailwind.config" -o -e "tailwind.config.js" -o -e "tailwind.config.ts" -o -e "tailwind.config.json"Repository: AOSSIE-Org/PictoPy
Length of output: 178
🏁 Script executed:
# Search for any Tailwind config references
rg -i "tailwind" --type json --type js --type ts -A 5 --max-count 20Repository: AOSSIE-Org/PictoPy
Length of output: 11629
🏁 Script executed:
find frontend -name "tailwind.config*" -type fRepository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Check if there's any tailwind config in frontend directory
ls -la frontend/ | grep -i tailwindRepository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Search for any custom spacing configuration in the codebase
rg "spacing|top-35|right-15" frontend/ --type ts --type js --type json -B 2 -A 2Repository: AOSSIE-Org/PictoPy
Length of output: 2063
🏁 Script executed:
# Check if there's a tailwind.config file anywhere in frontend or if it's using defaults
find frontend -type f \( -name "*.config.*" -o -name "tailwind.css" -o -name "globals.css" \) | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 135
🏁 Script executed:
# Check the App.css and index.css for any custom spacing definitions
cat frontend/src/App.css 2>/dev/null || echo "App.css not found"Repository: AOSSIE-Org/PictoPy
Length of output: 7941
🏁 Script executed:
# Search for CSS custom properties or theme overrides
rg "@layer|theme|--spacing|spacing:" frontend/src --type css -B 2 -A 2Repository: AOSSIE-Org/PictoPy
Length of output: 1700
Use valid Tailwind v4 spacing values or arbitrary value syntax for positioning.
top-35 and right-15 do not exist in Tailwind v4's default spacing scale (which includes only specific values: 0, px, 0.5, 1–12, 14, 16, 20, 24, 28, 32, 36, 40, 44, 48, 52, 56, 60, 64, 72, 80, 96). The project does not define custom spacing for these values. Use valid scale values like top-36 (9rem) or arbitrary values like top-[35px] and right-[15px] instead.
🤖 Prompt for AI Agents
In @frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx around lines
46 - 52, The Tailwind classes in ScrollToTopButton's className use invalid
spacing keys (top-35, right-15); update the positioning to use valid scale
values or arbitrary value syntax: replace top-35/right-15 in the className
string of the ScrollToTopButton component with either a nearest valid scale like
top-36/right-16 or explicit arbitrary values like top-[35px] and right-[15px] so
Tailwind v4 compiles correctly.
Add Floating “Back to Top” Button for Improved Navigation
This PR introduces a floating “Back to Top” button to PictoPy, making it easier for users to quickly return to the top of long pages without relying solely on the side scrollbar. The feature is designed to enhance usability while keeping the UI clean and unobtrusive.
Problem
Currently, PictoPy provides a side scrollbar for navigating between sections, which works well. However, on pages with many sections or large image collections, returning to the top requires moving the cursor to the first section of the scrollbar, which can feel inconvenient and slow.
Solution
A floating Back to Top button has been added to the bottom-right corner of the page.
The button becomes visible after the user scrolls down and allows smooth scrolling back to the top with a single click.
Benefits
Screenshots
Before
After
Screencast from 2026-01-07 22-21-45.webm
Changes
Scope
Additional Notes
This implementation follows existing UI patterns in the codebase and keeps the feature modular and reusable. The change improves usability without affecting performance or existing navigation behavior.
Code of Conduct
I agree to follow this project's Code of Conduct.
Contribution
I would like to work on this issue and contribute this improvement to PictoPy.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.