ui icon indicating copy to clipboard operation
ui copied to clipboard

feat(carousel): navigation dots

Open huri3l opened this issue 1 year ago • 12 comments

As I was using Carousel component, I needed to implement Dots Pagination in my project. They are pretty common in many Carousel/Slider libraries, such as slick, Keen-Slider, Swiper and etc.

This pull request adds a CarouselDots component to improve the current Carousel.

image

huri3l avatar Jan 18 '24 19:01 huri3l

@huri3l is attempting to deploy a commit to the shadcn-pro Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Jan 18 '24 19:01 vercel[bot]

Seems pretty solid and useful. I would love to see this merged.

jotelles avatar Jan 18 '24 19:01 jotelles

please approve this

lavnicki avatar Jan 18 '24 19:01 lavnicki

Please approve this

Chrystianpaim avatar Jan 18 '24 19:01 Chrystianpaim

bruh, my big head homie is a total genius, you just gotta approve it. please.

elcharrua avatar Jan 18 '24 20:01 elcharrua

Pagination with dots is a highly beneficial feature, especially when dealing with a substantial number of rendered cards. This implementation significantly enhances the user experience, allowing users to navigate directly to a specific card without the need to scroll through them one by one.

Great improvement!

Dunkode avatar Jan 18 '24 23:01 Dunkode

That problem is being solved but I got another issue: While we have even numbers of item it breaks on last slides. When we are already at last slide it shows wrong in dots as well as in arrows.

Solution for previous issue:

const { api } = useCarousel()
  // const isCurrent = (index: number) => index === api?.selectedScrollSnap()
  const length = api?.scrollSnapList().length ?? 1
  const [current, setCurrent] = React.useState(0)

  React.useEffect(() => {
    if (!api) return
    setCurrent(api.selectedScrollSnap())
    api.on("select", () => setCurrent(api.selectedScrollSnap()))
  }, [api])

current === index ? "bg-muted-foreground" : "bg-muted-foreground/40"

New issues found:

https://github.com/shadcn-ui/ui/assets/118199354/7eb11649-d2a4-458e-876b-3f3a2cf4e285

soham2k06 avatar Jan 19 '24 15:01 soham2k06

I intend to continue the issue until it is merged and available for everyone.

Thanks for the feedback, I'll do my best to fix the issues you found.

I loved the suggestion for dotsSize and dotsGap. Already working on that.

huri3l avatar Jan 19 '24 17:01 huri3l

About the issue that the carousel has an extra step, I have opened an issue into embla-carousel repository.

In the moment, I am not sure if the Carousel component from shadcn/ui is using it wrong, or if it is actually a bug existent on Embla Carousel.

As far as I have debugged, in the specific case of 6 contents and 3 visible per slide, embla-carousel is generating one extra point in the snapList that is really close to the last one.

image

And if you look closely, you can see the carousel actually moving in the last step.

So this solution will be handled through the issue linked above. If it is something that embla-carousel should fix, there is nothing else to be done. If there is something that needs to be changed in shadcn/ui, I will open another pull request fixing it in the future.

The goal is to see if embla-carousel can detect that the 3rd point is really close to the 4th point (in this example) to merge them into one. I suppose this is happening due to the fact that flex-basis is the one who rules how many content will be visible in each slide, and some values of the flex-basis are float, leading to this bug.

huri3l avatar Jan 21 '24 18:01 huri3l

@soham2k06 the gap and size props were added. Considering that David from embla-carousel is analysing the issue you have mentioned before, can we proceed with this Pull Request?

huri3l avatar Jan 21 '24 19:01 huri3l

@soham2k06 a bug fix for this has already been released in v8.0.0-rc20 👍🏻.

davidjerleke avatar Jan 23 '24 18:01 davidjerleke

@soham2k06 but we still need to bump the packages in this PR and someone also has to approve it I think:

  • #2286

Of course, if everything else has been solved in that pull request.

davidjerleke avatar Jan 23 '24 18:01 davidjerleke

Hey @huri3l,

After working on my own implementation of dot pagination for the Carousel component, I came across your PR and found some great ideas that complemented my approach. I've since refined my version to include a streamlined pagination logic within the useCarousel hook and flexible dot placement for different orientations.

Feel free to check it out https://github.com/shadcn-ui/ui/pull/3463 Perhaps there's an opportunity to combine our efforts.

Jacksonmills avatar Apr 11 '24 22:04 Jacksonmills

Hey @huri3l,

After working on my own implementation of dot pagination for the Carousel component, I came across your PR and found some great ideas that complemented my approach. I've since refined my version to include a streamlined pagination logic within the useCarousel hook and flexible dot placement for different orientations.

Feel free to check it out #3463 Perhaps there's an opportunity to combine our efforts.

Hi! I will check it out right now.

huri3l avatar Apr 15 '24 13:04 huri3l