Fix(carousel): Simplify scrolling logic to fix arrow buttons #2212
Conversation
Removed complex state management for the carousel's scroll position. Replaced with a direct and reliable scrollBy(clientWidth) call to ensure smooth and predictable navigation.
Removed complex state management for the carousel's scroll position. Replaced with a direct and reliable scrollBy(clientWidth) call to ensure smooth and predictable navigation. Ajusted the whitespace for code readability
appwrite.ioProject ID: Note You can use Avatars API to generate QR code for any text or URLs. |
|
The preview deployment is ready. 🟢 Open Preview | Open Build Logs Last updated at: 2025-07-24 08:51:02 CET |
WalkthroughThe carousel component removed its internal scroll amount calculation and related state. The next() and prev() functions now scroll horizontally by the container’s visible width using carousel.clientWidth and -carousel.clientWidth, rather than computing item-based paging. Existing scroll event handling and UI state logic remain intact. No changes were made to exported or public APIs. No new error handling was added. Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/components/Carousel.svelte (2)
52-56: Forward arrow is hard-disabled; button never becomes clickable.The forward button has a plain
disabledattribute, so it’s always disabled. Bind it toisEndlike the back button binds toisStart.Apply this diff:
- <button + <button class="web-icon-button cursor-pointer" aria-label="Move carousel forward" - disabled - onclick={next} + disabled={isEnd} + on:click={next} >
43-50: Use Svelte event bindings (on:event) instead of native attributes
- In
src/lib/components/Carousel.svelte, replace both handlers:- onclick={prev} + on:click={prev} - onclick={next} + on:click={next}
- A grep over the repo shows countless other uses of
onclick,onmouseover,onfocus, etc. All should be converted to Svelte syntax (on:event={handler}) to ensure handlers attach correctly. For example:rg -nP '(<\w+[^>]*\s)(on\w+)='
🧹 Nitpick comments (3)
src/lib/components/Carousel.svelte (3)
28-34: Initialize nav state on mount to avoid stale disabled states on first render.Until the first scroll event,
isStart/isEndmay be inaccurate. TriggerhandleScroll()on mount.Apply this diff:
<script lang="ts"> - import type { Snippet } from 'svelte'; + import type { Snippet } from 'svelte'; + import { onMount } from 'svelte'; @@ let isStart = $state(true); @@ function handleScroll() { isStart = carousel.scrollLeft <= 0; isEnd = Math.ceil(carousel.scrollLeft + carousel.offsetWidth) >= carousel.scrollWidth; } + + onMount(() => { + // sync initial state after bind:this resolves + handleScroll(); + }); </script>
4-4: Tighten the ref type.Since
bind:thistargets a UL, preferHTMLUListElementfor stronger typing.Apply this diff:
- let carousel: HTMLElement; + let carousel: HTMLUListElement;
15-26: Honor prefers-reduced-motion for smoother a11y.Avoid forced smooth scrolling for users who prefer reduced motion.
Apply this diff:
- function next() { - carousel.scrollBy({ - left: carousel.clientWidth, - behavior: 'smooth' - }); - } + function next() { + const behavior: ScrollBehavior = + typeof window !== 'undefined' && + window.matchMedia?.('(prefers-reduced-motion: reduce)').matches + ? 'auto' + : 'smooth'; + carousel.scrollBy({ left: carousel.clientWidth, behavior }); + } @@ - function prev() { - carousel.scrollBy({ - left: -carousel.clientWidth, - behavior: 'smooth' - }); - } + function prev() { + const behavior: ScrollBehavior = + typeof window !== 'undefined' && + window.matchMedia?.('(prefers-reduced-motion: reduce)').matches + ? 'auto' + : 'smooth'; + carousel.scrollBy({ left: -carousel.clientWidth, behavior }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/Carousel.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: tests
🔇 Additional comments (2)
src/lib/components/Carousel.svelte (2)
15-20: LGTM: Simpler page-wise scroll via clientWidth.Directly using scrollBy with clientWidth is a good simplification and should be more reliable than manual tracking.
21-26: LGTM: Symmetric prev() implementation.Matches next() logic and keeps behavior consistent.
Description of the Change
This pull request fixes a bug in the
Carousel.sveltecomponent where the arrow buttons were not working correctly.The original implementation used complex and unreliable state management to calculate the scroll distance. This fix simplifies the logic entirely by removing the manual scroll tracking and using a direct
carousel.scrollBy({ left: carousel.clientWidth })call. This approach is more robust, simpler to maintain, and ensures the carousel scrolls as expected.Local Build Environment
The contribution guide asks to run
pnpm run buildbefore submitting. Unfortunately, I was unable to get a successful build on my local Windows machine due to a persistentEMFILE: too many open fileserror, which is a known OS limitation with very large projects. I have made several attempts to resolve it (memory increase, clean installs) without success.As this change is a small, self-contained UI fix to a single component, I am confident it will not break the production build, which will be verified by the CI pipeline.
How to Test
/docspage.Contributor Checklist
pnpm run buildstep due to a local environment issue (EMFILE). I am relying on the CI to validate the build.mainbranch ofappwrite/website.Summary by CodeRabbit