-
-
Notifications
You must be signed in to change notification settings - Fork 503
fix(carousel): seamless circular transition between last and first slides #1669
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?
Changes from all commits
a75794e
1739c59
3c071e0
666bcf2
bda2185
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| "flowbite-react": patch | ||
| --- | ||
|
|
||
| fix(carousel): seamless circular transition between last and first slides | ||
|
|
||
| ### Changes | ||
|
|
||
| - [x] implement circular carousel using clone-based technique for smooth wrap-around | ||
| - [x] extract animation duration to named constant | ||
| - [x] prevent memory leak with timeout cleanup on unmount | ||
| - [x] guard initialization to prevent reset on dynamic children updates |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,8 @@ export interface CarouselProps extends ComponentProps<"div">, ThemingProps<Carou | |
|
|
||
| export interface DefaultLeftRightControlProps extends ComponentProps<"div">, ThemingProps<CarouselTheme> {} | ||
|
|
||
| const SCROLL_ANIMATION_DURATION_MS = 500; | ||
|
|
||
| export const Carousel = forwardRef<HTMLDivElement, CarouselProps>((props, ref) => { | ||
| const provider = useThemeProvider(); | ||
| const theme = useResolveTheme( | ||
|
|
@@ -89,8 +91,11 @@ export const Carousel = forwardRef<HTMLDivElement, CarouselProps>((props, ref) = | |
| const [activeItem, setActiveItem] = useState(0); | ||
| const [isDragging, setIsDragging] = useState(false); | ||
| const [isHovering, setIsHovering] = useState(false); | ||
| const [isAnimating, setIsAnimating] = useState(false); | ||
|
|
||
| const didMountRef = useRef(false); | ||
| const initializedRef = useRef(false); | ||
| const transitionTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
||
| const items = useMemo( | ||
| () => | ||
|
|
@@ -102,31 +107,102 @@ export const Carousel = forwardRef<HTMLDivElement, CarouselProps>((props, ref) = | |
| [children, theme.item.base], | ||
| ); | ||
|
|
||
| const itemCount = items?.length ?? 0; | ||
|
|
||
| const navigateTo = useCallback( | ||
| (item: number) => () => { | ||
| if (!items) return; | ||
| item = (item + items.length) % items.length; | ||
| if (carouselContainer.current) { | ||
| carouselContainer.current.scrollLeft = carouselContainer.current.clientWidth * item; | ||
| if (!items || isAnimating) return; | ||
|
|
||
| const container = carouselContainer.current; | ||
| if (!container) return; | ||
|
|
||
| const totalItems = items.length; | ||
| const targetItem = ((item % totalItems) + totalItems) % totalItems; | ||
|
|
||
| const isWrappingForward = activeItem === totalItems - 1 && item >= totalItems; | ||
| const isWrappingBackward = activeItem === 0 && item < 0; | ||
|
|
||
| if (isWrappingForward || isWrappingBackward) { | ||
| setIsAnimating(true); | ||
|
|
||
| if (transitionTimeoutRef.current) { | ||
| clearTimeout(transitionTimeoutRef.current); | ||
| } | ||
|
|
||
| // Scroll to the clone (last element for backward, first-after-last for forward) | ||
| if (isWrappingForward) { | ||
| container.scrollTo({ | ||
| left: container.clientWidth * (totalItems + 1), | ||
| behavior: "smooth", | ||
| }); | ||
| } else { | ||
| container.scrollTo({ | ||
| left: 0, | ||
| behavior: "smooth", | ||
| }); | ||
| } | ||
|
Comment on lines
+134
to
+143
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests fail because JSDOM lacks The static analysis shows multiple test failures with Add a mock in your test setup or at the top of the test file: 🧪 Proposed fix: Mock scrollTo in testsAdd to your test setup file or before tests: beforeAll(() => {
Element.prototype.scrollTo = vi.fn();
});Or if you need to verify scroll positions: beforeEach(() => {
Element.prototype.scrollTo = vi.fn(function(this: Element, options?: ScrollToOptions) {
if (options?.left !== undefined) {
(this as any).scrollLeft = options.left;
}
});
});Also applies to: 160-166 🤖 Prompt for AI Agents |
||
|
|
||
| // After the smooth scroll animation, jump instantly to the real slide | ||
| const onTransitionDone = () => { | ||
| container.style.scrollBehavior = "auto"; | ||
| if (isWrappingForward) { | ||
| container.scrollLeft = container.clientWidth * 1; | ||
| } else { | ||
| container.scrollLeft = container.clientWidth * totalItems; | ||
| } | ||
| container.style.scrollBehavior = ""; | ||
| setIsAnimating(false); | ||
| transitionTimeoutRef.current = null; | ||
| }; | ||
|
|
||
| transitionTimeoutRef.current = setTimeout(onTransitionDone, SCROLL_ANIMATION_DURATION_MS); | ||
| setActiveItem(targetItem); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } else { | ||
| // Normal navigation - account for the prepended clone | ||
| container.scrollTo({ | ||
| left: container.clientWidth * (targetItem + 1), | ||
| behavior: "smooth", | ||
| }); | ||
| setActiveItem(targetItem); | ||
| } | ||
| setActiveItem(item); | ||
| }, | ||
| [items], | ||
| [items, activeItem, isAnimating], | ||
| ); | ||
|
|
||
| // Initialize scroll position to first real slide (past the prepended clone) | ||
| useEffect(() => { | ||
| const container = carouselContainer.current; | ||
| if (container && items && items.length > 0 && !initializedRef.current) { | ||
| initializedRef.current = true; | ||
| container.style.scrollBehavior = "auto"; | ||
| container.scrollLeft = container.clientWidth * 1; | ||
| container.style.scrollBehavior = ""; | ||
| } | ||
| }, [items]); | ||
|
|
||
| useEffect(() => { | ||
| if (carouselContainer.current && !isDragging && carouselContainer.current.scrollLeft !== 0) { | ||
| setActiveItem(Math.round(carouselContainer.current.scrollLeft / carouselContainer.current.clientWidth)); | ||
| if (carouselContainer.current && !isDragging && !isAnimating) { | ||
| const container = carouselContainer.current; | ||
| const rawIndex = Math.round(container.scrollLeft / container.clientWidth); | ||
| // Account for the prepended clone: real items start at index 1 | ||
| const totalItems = items?.length ?? 0; | ||
| if (totalItems > 0) { | ||
| const realIndex = (((rawIndex - 1) % totalItems) + totalItems) % totalItems; | ||
| setActiveItem(realIndex); | ||
| } | ||
| } | ||
| }, [isDragging]); | ||
| }, [isDragging, isAnimating, items]); | ||
|
|
||
| useEffect(() => { | ||
| if (slide && !(pauseOnHover && isHovering)) { | ||
| const intervalId = setInterval(() => !isDragging && navigateTo(activeItem + 1)(), slideInterval ?? 3000); | ||
| const intervalId = setInterval( | ||
| () => !isDragging && !isAnimating && navigateTo(activeItem + 1)(), | ||
| slideInterval ?? 3000, | ||
| ); | ||
|
|
||
| return () => clearInterval(intervalId); | ||
| } | ||
| }, [activeItem, isDragging, navigateTo, slide, slideInterval, pauseOnHover, isHovering]); | ||
| }, [activeItem, isDragging, isAnimating, navigateTo, slide, slideInterval, pauseOnHover, isHovering]); | ||
|
|
||
| useEffect(() => { | ||
| if (didMountRef.current) { | ||
|
|
@@ -136,11 +212,55 @@ export const Carousel = forwardRef<HTMLDivElement, CarouselProps>((props, ref) = | |
| } | ||
| }, [onSlideChange, activeItem]); | ||
|
|
||
| // Cleanup transition timeout on unmount | ||
| useEffect(() => { | ||
| return () => { | ||
| if (transitionTimeoutRef.current) { | ||
| clearTimeout(transitionTimeoutRef.current); | ||
| } | ||
| }; | ||
| }, []); | ||
|
|
||
| const handleDragging = (dragging: boolean) => () => setIsDragging(dragging); | ||
|
|
||
| // Handle drag end: snap to nearest real slide and fix wrap-around | ||
| const handleDragEnd = useCallback(() => { | ||
| setIsDragging(false); | ||
| const container = carouselContainer.current; | ||
| if (!container || !items) return; | ||
|
|
||
| const totalItems = items.length; | ||
| const rawIndex = Math.round(container.scrollLeft / container.clientWidth); | ||
|
|
||
| // If scrolled to the prepended clone (index 0), jump to real last slide | ||
| if (rawIndex <= 0) { | ||
| container.style.scrollBehavior = "auto"; | ||
| container.scrollLeft = container.clientWidth * totalItems; | ||
| container.style.scrollBehavior = ""; | ||
| setActiveItem(totalItems - 1); | ||
| } | ||
| // If scrolled to the appended clone (index totalItems + 1), jump to real first slide | ||
| else if (rawIndex >= totalItems + 1) { | ||
| container.style.scrollBehavior = "auto"; | ||
| container.scrollLeft = container.clientWidth * 1; | ||
| container.style.scrollBehavior = ""; | ||
| setActiveItem(0); | ||
| } else { | ||
| setActiveItem(rawIndex - 1); | ||
| } | ||
| }, [items]); | ||
|
|
||
| const setHoveringTrue = useCallback(() => setIsHovering(true), []); | ||
| const setHoveringFalse = useCallback(() => setIsHovering(false), []); | ||
|
|
||
| // Build the extended items array: [cloneLast, ...items, cloneFirst] | ||
| const extendedItems = useMemo(() => { | ||
| if (!items || items.length === 0) return items; | ||
| const lastClone = cloneElement(items[items.length - 1], { key: "clone-last" }); | ||
| const firstClone = cloneElement(items[0], { key: "clone-first" }); | ||
| return [lastClone, ...items, firstClone]; | ||
| }, [items]); | ||
|
|
||
| return ( | ||
| <div | ||
| ref={ref} | ||
|
|
@@ -156,16 +276,16 @@ export const Carousel = forwardRef<HTMLDivElement, CarouselProps>((props, ref) = | |
| className={twMerge(theme.scrollContainer.base, (isDeviceMobile || !isDragging) && theme.scrollContainer.snap)} | ||
| draggingClassName="cursor-grab" | ||
| innerRef={carouselContainer} | ||
| onEndScroll={handleDragging(false)} | ||
| onEndScroll={handleDragEnd} | ||
| onStartScroll={handleDragging(draggable)} | ||
| vertical={false} | ||
| horizontal={draggable} | ||
| > | ||
| {items?.map((item, index) => ( | ||
| {extendedItems?.map((item, index) => ( | ||
| <div | ||
| key={index} | ||
| className={theme.item.wrapper[draggable ? "on" : "off"]} | ||
| data-active={activeItem === index} | ||
| data-active={activeItem === (index - 1 + itemCount) % itemCount} | ||
| data-testid="carousel-item" | ||
| > | ||
| {item} | ||
|
|
||
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.
realCarouselItems()is brittle and is likely causing the current failing assertions.The helper always strips the first/last nodes, which breaks when clones are not present (or not represented in queried items for that render state). That matches the failing expectations on Line 25, Line 82, Line 100, and Line 118.
🔧 Proposed fix
const carouselItems = () => screen.getAllByTestId("carousel-item"); // Real items exclude the prepended and appended clones const realCarouselItems = () => { const items = carouselItems(); - return items.slice(1, items.length - 1); + const indicators = screen.queryAllByTestId("carousel-indicator"); + const hasBoundaryClones = indicators.length > 0 && items.length === indicators.length + 2; + return hasBoundaryClones ? items.slice(1, items.length - 1) : items; };🤖 Prompt for AI Agents