feat(ui): adding carousel component#399
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughAdds a new Swiper-based Carousel component with types and re-exports, Storybook stories demonstrating many configurations, and updates Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Consumer
participant Carousel
participant Swiper
participant Modules
Consumer->>Carousel: pass props (slides, showNavigation, showPagination, startingSlideIndex, noSwipingSelector, modules, ...)
Carousel->>Modules: assemble default modules (A11y, Keyboard) + conditional (Navigation, Pagination) + extra modules
Carousel->>Swiper: initialize with composed modules, props, startingSlideIndex
Swiper->>Swiper: manage behavior (keyboard onlyInViewport, loop, autoplay, pagination/nav)
Carousel->>Carousel: render slides as SwiperSlide nodes
Swiper->>Consumer: interactive carousel UI rendered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
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
🧹 Nitpick comments (3)
packages/ui/src/components/Carousel/Carousel.types.ts (1)
1-11: CarouselProps shape looks good; consider minor typing polishThis props interface cleanly wraps
SwiperPropswhile exposing the extra carousel options. A couple of optional tweaks you might consider:
- Make
slidesReadonlyArray<React.ReactNode>so callers can safely pass both mutable and readonly arrays while signalling you won’t mutate them.- Since
modulesandinitialSlideare important to how the wrapper behaves inCarousel.tsx, you might explicitly override them here (e.g.modules?: SwiperProps['modules'], orOmit<SwiperProps, 'children' | 'initialSlide'>) so the generated/IDE docs make the intended usage and precedence clearer.These are non-blocking; the current type is perfectly usable.
packages/ui/src/components/Carousel/Carousel.tsx (1)
1-48: Solid Swiper wrapper; watch prop precedence and configuration overlapThe implementation looks correct and matches Swiper’s React API; keyboard/a11y modules by default and the conditional Navigation/Pagination modules are a nice touch.
A few small, mostly optional points:
startingSlideIndexvsinitialSlideprecedence
Because...swiperPropsis spread last, any consumer-providedinitialSlidewill silently overridestartingSlideIndex. If you intendstartingSlideIndexto be the single source of truth, consider omittinginitialSlidefromCarouselProps(e.g.Omit<SwiperProps, 'children' | 'initialSlide'>) so TS prevents the conflicting prop.Modules duplication
allModulesalways includesA11yandKeyboard, then conditionallyNavigation/Pagination, then any user-suppliedmodules. If a caller also addsNavigation/Paginationinmodules, they’ll get duplicates. Swiper copes with this, but you may want to document thatmodulesshould not repeat those, or defensivelynew Set([...])the list if you ever run into side effects.CSS import location
Importing Swiper CSS inside the component file is convenient but makes this module have side effects. That’s fine for an app-level UI package, but if you ever care about stricter tree‑shaking or giving host apps control over CSS loading, an alternative is to re-export aglobals.css/theme entry and document that consumers must import it once.None of these are blockers; behavior as written is coherent.
packages/ui/src/components/Carousel/Carousel.stories.tsx (1)
1-174: Story coverage is strong; only minor Storybook polish possibleThese stories do a nice job exercising the full Carousel API (navigation, pagination, responsiveness, autoplay, looping, starting index,
noSwipingSelector), which will be very helpful for both testing and documentation.A couple of non-blocking nits:
- The
keyprops on<SampleSlide>elements are redundant sinceCarouselremapsslidesinto<SwiperSlide key={index}>internally; you can drop them in the story data if you like.- For
SampleSlide, you could move the fixed height from inlinestyleinto Tailwind classes or a small CSS helper for consistency with the rest of the design system, though for Storybook-only content this is more style than substance.Functionally, everything looks aligned with the Swiper API (including
modules: [Autoplay]and the autoplay options).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
packages/ui/package.json(2 hunks)packages/ui/src/components/Carousel/Carousel.stories.tsx(1 hunks)packages/ui/src/components/Carousel/Carousel.tsx(1 hunks)packages/ui/src/components/Carousel/Carousel.types.ts(1 hunks)packages/ui/src/components/Carousel/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/ui/src/components/Carousel/Carousel.types.ts (2)
packages/ui/src/components/Carousel/index.ts (1)
CarouselProps(2-2)packages/integrations/strapi-cms/generated/strapi.ts (1)
Omit(14-14)
packages/ui/src/components/Carousel/Carousel.stories.tsx (1)
packages/ui/src/components/Carousel/Carousel.tsx (1)
Carousel(14-48)
packages/ui/src/components/Carousel/Carousel.tsx (3)
packages/ui/src/components/Carousel/index.ts (2)
Carousel(1-1)CarouselProps(2-2)packages/ui/src/components/Carousel/Carousel.types.ts (1)
CarouselProps(4-11)packages/ui/src/lib/utils.ts (1)
cn(5-7)
🪛 GitHub Actions: Preview Deployment
packages/ui/package.json
[error] 1-1: npm ci failed: lockfile and package.json are out of sync. Run 'npm install' to update the lockfile before continuing. Exit code 1.
🔇 Additional comments (2)
packages/ui/src/components/Carousel/index.ts (1)
1-2: Re-exports correctly expose the Carousel public APIRe-exporting both the component and its props from the local barrel keeps consumers on a single import path and matches the underlying named exports.
packages/ui/package.json (1)
31-74: Dependencies align correctly; lockfile appears synchronizedThe added/updated entries (
@types/throttle-debounce,typescript@^5.9.3,react-day-picker,swiper@^12.0.3) are sound and consistent with the new Carousel feature. Package versions already align across the monorepo (TypeScript^5.9.3is used in ~40+ packages; Swiper only inpackages/ui).No action required — the lockfile is present and synchronized with package.json. Swiper 12.0.3 and TypeScript 5.9.3 are compatible with standard tsconfig settings.
Likely an incorrect or invalid review comment.
marcinkrasowski
left a comment
marcinkrasowski
left a comment
There was a problem hiding this comment.
please run npm run changeset so that the changelog in the ui package is updated with the new component
… feature/carousel-component
… feature/carousel-component
2 participants
What does this PR do?
Key Changes
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.