fix: Carousel navigation buttons and scrolling functionality #2326
Conversation
Signed-off-by: fyzanshaik <[email protected]>
|
The preview deployment failed. 🔴 Last updated at: 2025-08-27 20:38:32 CET |
appwrite.ioProject ID: Note Appwrite has a Discord community with over 16 000 members. |
Signed-off-by: fyzanshaik <[email protected]>
Walkthrough
Possibly related issues
Pre-merge checks (5 passed)✅ Passed checks (5 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: 1
🧹 Nitpick comments (3)
src/lib/components/Carousel.svelte (3)
47-51: Harden end detection to avoid off-by-a-few-pixels issues across zoom/scrollbar variances.Use a small epsilon and
clientWidthto reduce false negatives/positives at boundaries.- isStart = carousel.scrollLeft <= 0; - isEnd = Math.ceil(carousel.scrollLeft + carousel.offsetWidth) >= carousel.scrollWidth - 1; + const epsilon = 2; // px tolerance + isStart = carousel.scrollLeft <= epsilon; + isEnd = carousel.scrollLeft + carousel.clientWidth >= carousel.scrollWidth - epsilon;
53-59: Avoid scheduling unbounded timeouts on every state change; initialize on mount and react to resizes.
$effectruns after any state change; each run schedules asetTimeout, which can pile up. PreferonMountand observe size changes to keepisStart/isEndaccurate on responsive layouts.-$effect(() => { - if (carousel) { - setTimeout(() => { - handleScroll(); - }, 0); - } -}); +onMount(() => { + handleScroll(); + const ro = new ResizeObserver(() => handleScroll()); + if (carousel) ro.observe(carousel); + const onWinResize = () => handleScroll(); + window.addEventListener('resize', onWinResize); + return () => { + ro.disconnect(); + window.removeEventListener('resize', onWinResize); + }; +});Add (outside this hunk) at the top:
import { onMount } from 'svelte';Want me to include a
MutationObserveras well to react when items mount/unmount?
95-100: Wheel UX: let the page scroll at edges, handle non-pixel deltaModes, and use scrollBy.Currently vertical wheel is always intercepted, which traps page scroll when the carousel is at start/end. Also, some devices emit line/page deltas. The tweak below improves ergonomics without changing intent.
- onwheel={(e) => { - if (e.deltaY !== 0) { - e.preventDefault(); - carousel.scrollLeft += e.deltaY; - } - }} + onwheel={(e) => { + if (!carousel) return; + const dy = e.deltaY; + if (dy === 0) return; + const atStart = carousel.scrollLeft <= 0; + const atEnd = + Math.ceil(carousel.scrollLeft + carousel.clientWidth) >= carousel.scrollWidth - 1; + // Let page scroll if we'd go beyond bounds + if ((dy < 0 && atStart) || (dy > 0 && atEnd)) return; + e.preventDefault(); + const multiplier = e.deltaMode === 1 ? 16 : e.deltaMode === 2 ? carousel.clientHeight : 1; + carousel.scrollBy({ left: dy * multiplier, behavior: 'auto' }); + }}Please verify across:
- Mac trackpad
- Standard mouse wheel on Windows/Linux
- High zoom (125%/150%) where fractional pixels are common
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/Carousel.svelte(4 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 (1)
src/lib/components/Carousel.svelte (1)
79-79: Good fix: forward button now respects scroll state.Binding
disabledtoisEndresolves the hardcoded-disabled bug.
| function calculateScrollAmount(prev = false) { | ||
| if (!carousel) return 0; | ||
| const direction = prev ? -1 : 1; | ||
| const carouselSize = carousel?.clientWidth; | ||
| const childSize = (carousel.childNodes[0] as HTMLUListElement)?.clientWidth + gap; | ||
| const carouselSize = carousel.clientWidth; | ||
| scroll = scroll || carouselSize; | ||
| const firstChild = carousel.querySelector('li') as HTMLElement; | ||
| if (!firstChild) return 0; | ||
| const childSize = firstChild.offsetWidth + gap; | ||
| const numberOfItems = Math.floor(carouselSize / childSize); | ||
| const overflow = scroll % childSize; | ||
| const amount = numberOfItems * childSize - overflow * direction; | ||
| scroll += amount * direction; | ||
| return amount * direction; | ||
| return numberOfItems * childSize * direction; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix zero-step bug when item is wider than viewport; prefer direct-child query and clamp page size.
If firstChild.offsetWidth + gap > carousel.clientWidth, Math.floor(...) becomes 0 so scrollBy({ left: 0 }) does nothing and buttons appear broken on narrow viewports. Also, querySelector('li') may match nested list items; use :scope > li.
- const firstChild = carousel.querySelector('li') as HTMLElement;
+ const firstChild = carousel.querySelector(':scope > li') as HTMLElement | null;
if (!firstChild) return 0;
- const childSize = firstChild.offsetWidth + gap;
- const numberOfItems = Math.floor(carouselSize / childSize);
-
- return numberOfItems * childSize * direction;
+ const childSize = firstChild.offsetWidth + gap;
+ const itemsPerPage = Math.max(1, Math.floor(carouselSize / childSize));
+ return itemsPerPage * childSize * direction;Optional typings outside this hunk (helps TS and DOM correctness):
// line 4
let carousel: HTMLUListElement;
// line 15
function calculateScrollAmount(prev = false): number {🤖 Prompt for AI Agents
In src/lib/components/Carousel.svelte around lines 15 to 28, the calculation can
return 0 when an item (offsetWidth + gap) is larger than the carousel viewport
and querySelector('li') may match nested items; update the function to select
direct children via carousel.querySelector(':scope > li') and compute
numberOfItems as Math.max(1, Math.floor(carouselSize / childSize)) (so we always
scroll at least one item), ensure the function returns a number type, and
optionally add typing for carousel as HTMLUListElement and the function
signature as calculateScrollAmount(prev = false): number.
2 participants
What does this PR do?
Fixes Carousel component where navigation buttons were disabled and users couldn't scroll.
Issues fixed:
disabledTest Plan
/docspageRelated PRs and Issues
Fixes #2122
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
New Features
Bug Fixes