Feat: added autoplay function to carousel #3183
Conversation
WalkthroughThe Carousel component adds optional autoplay controls: configurable interval, external pause state management (isPaused/setIsPaused), external current-page setter (setCurrentPage), and hover-to-pause handlers. A useEffect manages the autoplay timer and cleans up on unmount or when paused/conditions change. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Carousel
participant Timer
participant ExternalState as setCurrentPage/setIsPaused
User->>Carousel: hover enter
Carousel->>ExternalState: setIsPaused(true)
Timer-->>Carousel: tick (if autoPlay && !isPaused)
Carousel->>ExternalState: setCurrentPage(nextPage)
User->>Carousel: hover leave
Carousel->>ExternalState: setIsPaused(false)
Note over Carousel,Timer: useEffect starts/stops Timer based on props and isPaused
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
rodrigoide-whirlpool
requested review from
lariciamota and
lemagnetic
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/components/src/molecules/Carousel/Carousel.tsx`:
- Around line 175-181: The autoplay effect in Carousel stops on the last slide
unconditionally; update the condition inside the useEffect (the block using
isPaused, autoPlay, sliderState.currentPage, and childrenCount) so it only
prevents autoplay when infiniteMode is false—i.e., change the check to include a
guard like "&& !infiniteMode" around the currentPage comparison—and add
infiniteMode to the effect's dependency array so the timer reacts to changes in
infiniteMode.
- Around line 170-193: The useEffect controlling autoplay is missing
dependencies and can call setInterval with an undefined delay; update the
dependency array to include autoPlay, autoPlayIntervalTime, childrenCount,
slide, sliderDispatch, and setCurrentPage so closures see fresh values, and add
a guard before creating the interval in the useEffect (e.g., ensure autoPlay is
true and typeof autoPlayIntervalTime === 'number' and > 0 and
sliderState.currentPage !== childrenCount - 1) so setInterval never receives
undefined; also ensure the cleanup still dispatches STOP_SLIDE via
sliderDispatch and clears the interval when created.
🧹 Nitpick comments (3)
packages/components/src/molecules/Carousel/Carousel.tsx (3)
91-104: Add JSDoc comments for new props.The new props lack JSDoc documentation, which is inconsistent with the existing well-documented props in this interface.
autoPlayIntervalTimeespecially needs documentation specifying the unit (milliseconds).📝 Suggested documentation
- /* - * Check whether the carousel will play automatically or not. - */ - autoPlay?: boolean - autoPlayIntervalTime?: number - /* - * Specify when it's paused after mouse entering carousel items list. - */ - isPaused?: boolean - setIsPaused?: (paused: boolean) => void - /* - * Specify in which page carousel is at the moment. - */ - setCurrentPage?: (page: number) => void + /** + * Enables automatic advancement of carousel slides. + * `@default` false + */ + autoPlay?: boolean + /** + * Time interval between automatic slide transitions in milliseconds. + * `@default` 3000 + */ + autoPlayIntervalTime?: number + /** + * Controls whether autoplay is currently paused. + */ + isPaused?: boolean + /** + * Callback to update the paused state (e.g., on hover). + */ + setIsPaused?: (paused: boolean) => void + /** + * Callback invoked with the current page index when it changes. + */ + setCurrentPage?: (page: number) => void
442-449: Accessibility: Pause-on-hover alone is insufficient for WCAG compliance.Hover-based pausing doesn't help keyboard users or those with motor impairments (WCAG 2.2.2). Consider adding:
- A visible pause/play button
- Pausing when any carousel control receives focus
aria-live="off"when autoplay is active to reduce announcementsFor now, at minimum, consider pausing on focus within the track:
{...(isPaused !== undefined && setIsPaused !== undefined ? { onMouseEnter: () => { setIsPaused(true) }, onMouseLeave: () => { setIsPaused(false) - } + }, + onFocus: () => { + setIsPaused(true) + }, + onBlur: (e: React.FocusEvent) => { + if (!e.currentTarget.contains(e.relatedTarget)) { + setIsPaused(false) + } + } } : {})}
435-435: Consideraria-live="off"during autoplay.The track currently has
aria-live="polite", which will announce every slide change during autoplay—potentially overwhelming for screen reader users. Toggle to"off"when autoplay is active.<ul - aria-live="polite" + aria-live={autoPlay && !isPaused ? 'off' : 'polite'} ref={carouselTrackRef}
…d useEffect dependency array incomplete
1 participant
What's the purpose of this pull request?
The purpose is adding the possibility to the carousel to play automatically.
How it works?
It checks if the autoplay prop is true, then it sets an interval following the autoPlayIntervalTime prop, it'll also pause the autoplay whenever the user hover over the carousel items list.
How to test it?
For it to be tested, you should call the carousel molecule from @faststore/ui, then add the props autoPlay and autoPlayIntervalTime, then if prefer, add states to prevent autoplay to keep going when mouse enters it's carousel list.
I also added a state function to get the current page, which is more specific for what is needed.
Starters Deploy Preview
In Progress...
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.