Skip to content
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

Add carousel component #219

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lsouoliveira
Copy link
Contributor

@lsouoliveira lsouoliveira commented Jan 30, 2025

EDIT:

I removed the CarouselContext class. Now, the orientation-based styling of each component is set using the group-[.is-horizontal]: and group-[.is-vertical]: Tailwind selectors.

I also added keyboard events to mimic the shadcn component. When the right arrow key is pressed, the carousel moves to the next slide, and when the left arrow key is pressed, it moves to the previous slide.

ORIGINAL:
This PR adds a Carousel component based on the shadcn/ui

New JavaScript Dependency

The shadcn/ui Carousel component uses the Embla library to build the carousel, so I had to add it to the project. The Embla library allows users to add plugins during API initialization. Ruby ui users can add plugins by modifying the ruby-ui--carousel controller. New plugins can be passed to the #initCarousel method.

Use of Thread.current

The carousel supports two orientations: horizontal and vertical. Each orientation affects the classes of all Carousel child components (CarouselContent, CarouselItem, CarouselPrevious, and CarouselNext).

Initially, I added an orientation parameter to each child component, but this led to a lot of repetition when rendering the carousel:

image

To address this, I created a CarouselContext class that wraps the root component (Carousel). CarouselContext uses Thread.current to set a local variable that all components inside the Carousel block can access. This new approach allowed me to eliminate parameter repetition:

image

Demonstration

showcase.mp4

@lsouoliveira lsouoliveira force-pushed the add-carousel-component branch 2 times, most recently from 887ff8d to 1fe3b66 Compare January 31, 2025 02:03
@lsouoliveira lsouoliveira force-pushed the add-carousel-component branch from 1fe3b66 to 002845b Compare January 31, 2025 02:13
@cirdes
Copy link
Collaborator

cirdes commented Feb 16, 2025

@lsouoliveira , thanks for the component. It looks nice. The Embla library is the same that shadcn uses, and it's fair to use it as a dependency.

The only thing that makes me feel uncomfortable is the use of Thread to share data. I might not work with turbo frames.

You might achieve a similar result using Tailwind groups. I can mark the parent as is-horizontal and apply a CSS rule based on that. Or try to use stimulus with tailwind to do the job

Any inputs here? @stephannv ?

@lsouoliveira
Copy link
Contributor Author

@cirdes Thanks for the feedback. Your concerns about Thread.current are definitely valid. Tailwind groups should work just as well and are much simpler.

@stephannv
Copy link
Contributor

@lsouoliveira , thanks for the component. It looks nice. The Embla library is the same that shadcn uses, and it's fair to use it as a dependency.

The only thing that makes me feel uncomfortable is the use of Thread to share data. I might not work with turbo frames.

You might achieve a similar result using Tailwind groups. I can mark the parent as is-horizontal and apply a CSS rule based on that. Or try to use stimulus with tailwind to do the job

Any inputs here? @stephannv ?

Yeah, I prefer the tailwind (best) or stimulus solution. I faced a similar situation with combobox component that could be solved with Thread.current but I didn't like the solution at the time.

@lsouoliveira lsouoliveira force-pushed the add-carousel-component branch from 5c6daea to 1dcd8c9 Compare February 17, 2025 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants