feat: improved the scroll to top button #3616
Conversation
hkv24
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
❌ Deploy Preview for asyncapi-website failed.Built without sensitive environment variables
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3616 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 732 732
=========================================
Hits 732 732 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
components/buttons/ScrollButton.tsx (1)
Line range hint
13-19: Add cleanup for scroll event listener.The scroll event listener is not cleaned up when the component unmounts, which could lead to memory leaks. Add a cleanup function in the useEffect hook.
useEffect(() => { - window.addEventListener('scroll', () => { + const handleScroll = () => { if (window.scrollY > 150) { setBackToTopButton(true); } else { setBackToTopButton(false); } - }); + }; + + window.addEventListener('scroll', handleScroll); + return () => window.removeEventListener('scroll', handleScroll); }, []);
🧹 Nitpick comments (1)
components/buttons/ScrollButton.tsx (1)
36-36: Consider optimizing the Image component configuration.While the Image component implementation is correct, consider these optimizations:
- Since you're using an SVG, you might not need the Next.js Image component. SVGs are already optimized vector graphics.
- If you keep the Image component, consider adding
loading="lazy"since this is below-the-fold content.- <Image src={scrollImage} alt='scroll to top' height={30} width={30} /> + <img src={scrollImage} alt='scroll to top' className="h-[30px] w-[30px]" />Or if you prefer keeping the Image component:
- <Image src={scrollImage} alt='scroll to top' height={30} width={30} /> + <Image + src={scrollImage} + alt='scroll to top' + height={30} + width={30} + loading="lazy" + />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/img/loaders/scroll.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
components/buttons/ScrollButton.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (2)
components/buttons/ScrollButton.tsx (2)
1-1: LGTM! Good use of Next.js Image component.Using
next/imageinstead of the standardimgtag will provide automatic image optimization, lazy loading, and prevent Cumulative Layout Shift (CLS).
30-35: Verify mobile compatibility with reduced bottom spacing.The button's position has been moved closer to the bottom (from bottom-14 to bottom-4). While the layout improvements with flex and sizing look good, please verify that:
- The button doesn't overlap with mobile browser navigation bars
- The button remains easily tappable on mobile devices (consider touch target size guidelines)
✅ Verification successful
Mobile compatibility verified - implementation is safe ✅
The scroll button implementation:
- Has sufficient spacing (16px) from bottom edge
- Uses appropriate z-index layering to prevent conflicts
- Meets touch target size requirements (40x40px)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any mobile-specific styles or media queries that might affect the button's positioning rg -g '*.css' -g '*.scss' -g '*.tsx' -g '*.jsx' 'bottom-\d+.*mobile|@media.*bottom-\d+'Length of output: 89
Script:
#!/bin/bash # Search for mobile navigation components and bottom-fixed elements echo "=== Searching for mobile navigation components ===" rg -g '*.tsx' -g '*.jsx' 'nav|navbar|navigation' -A 5 -B 5 echo "=== Searching for fixed positioning and z-index usage ===" rg -g '*.tsx' -g '*.jsx' 'fixed.*bottom|z-\d+.*fixed|bottom.*z-\d+' echo "=== Checking viewport-related utilities ===" rg -g '*.tsx' -g '*.jsx' '@media|useMediaQuery|screen.*min-width|screen.*max-width'Length of output: 41148
|
closing this as not relevant |
Description
<img>tag with<Image />component to enhance the performance which will be done by Next JS automatically.Resolves #3614Summary by CodeRabbit
Imagecomponent