brainhubeu/react-carousel

Do you want to work on this issue?

You can request for a bounty in order to promote it!

Wrong active dots calculation #172

pierzchalatomasz posted onGitHub

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior:

  1. Create a carousel with 5 items
  2. Pass following props:
    <Carousel
    ...
    dots
    slidesPerPage={2}
    slidesPerScroll={2}
    >
  3. Keep changing active slides
  4. Notice the order of active dots (it's repeatable 1, 3, 5, 2, 4)

Expected behavior Probably active dots should be calculated differently for this kind of situation.

Screenshots ezgif com-video-to-gif (1)

Desktop (please complete the following information):

  • OS: [macOS 10.14.6]
  • Browser [Chrome]
  • Version [79.0.3945.88]

@piotr-s-brainhub has funded $5.00 to this issue.


posted by issuehunt-app[bot] almost 5 years ago

Note for others, the infinite and arrows props are vital for the reproduction.

<Carousel
  arrows={true}
  dots={true}
  infinite={true}
  slidesPerPage={2}
  slidesPerScroll={2}
  value={...}
  slides={...}
  onChange={...}
/>

The issue is really "How to represent dots when the carousel is infinite" πŸ€”

The current implementation seems correct, the "active" slide is the most lefthand and the active sequence 1, 3, 5, 2, 4 would be expected.

@pierzchalatomasz How would you expect this interaction to be represented?

posted by jamsinclair almost 5 years ago

Hi @jamsinclair,

In this situation we have 5 scenes (I mean slides visible at one time) in the following order (where [1, 2] meaning scene with slides 1 and 2 etc.):

  1. [1, 2]
  2. [3, 4]
  3. [5, 1]
  4. [2, 3]
  5. [4, 5]

I would expect that dots are calculated based on the index of scene instead of the first item in a current scene i.e. dots should be activated in the following order: 1 => 2 => 3 => 4 => 5 instead of 1 => 3 => 5 => 2 => 4.

posted by pierzchalatomasz almost 5 years ago

@pierzchalatomasz Thanks for the great explanation πŸ‘Œ . Interesting problem πŸ€”.

posted by jamsinclair almost 5 years ago

@pierzchalatomasz After merging https://github.com/brainhubeu/react-carousel/pull/177, the order is the following:

1: [1, 2]
2: [2, 3]
3: [3, 4]
4: [4, 5]
5: [5]

ezgif com-video-to-gif (1) So I would change only the end to be:

5: [5, 1]

What do you think?

posted by piotr-s-brainhub almost 5 years ago

@piotr-s-brainhub 5: [5, 1] instead of 5: [5, empty-space] makes sense if infinite is on. But the problem @pierzchalatomasz described is that when I have slidesPerPage={2}, slidesPerScroll={2} and 6 "slides", the actual number of pages is 3, so some users expect to have 3 dots. The issue here is that we always use the slide index and not page index. The reason for this is that we can have slidesPerPage={2} but slidesPerScroll={3}. What number of "pages" are there then? Not 3, because after clicking right arrow you end up on "page" 2,5. You can also have slidesPerPage={2} and controlled carousel, and pass the index 1 or 3 to it. You would be on a half page then.

I would leave the indexing as is, and the users just need to implement dots by themselves to have the behaviour @pierzchalatomasz suggested.

posted by Lukasz-pluszczewski almost 5 years ago

@Lukasz-pluszczewski do you mean this situation or I have some incorrect params?

<Carousel
  slidesPerPage={2}
  slidesPerScroll={2}
  infinite
  dots
>
 <div>foo-1</div>
 <div>foo-2</div>
 <div>foo-3</div>
 <div>foo-4</div>
 <div>foo-5</div>
</Carousel>

for 5 slides: ezgif com-video-to-gif (1)

for 6 slides: ezgif com-video-to-gif (2)

posted by piotr-s-brainhub almost 5 years ago

@piotr-s-brainhub has cancelled funding for this issue.(Cancelled amount: $5.00) See it on IssueHunt

posted by issuehunt-app[bot] over 4 years ago

I think that we should divide the amount of dots by slidesPerScroll by default. We may also add props to dots to control it.

posted by humbak over 4 years ago

I did a research and indeed this is an important bug.

We should correct the dots' behavior. How it should work:

carousel-dots

  1. number of dots should be equal to the number of slides divided by slidesPerScroll property and rounded up
  2. if we have the rest from division. On the action (e.g. clicking the arrow, drag the slide) on the last slides, we should scroll only the remaining ones.
  3. if we have the rest from dividing and infinite mode we should behave in the same way like in point 2
posted by humbak over 4 years ago

So...this is a major problem, I get it (and agree).

Any update on that? This is pretty important stuff... Any workaround that makes it work?

The problem really seems to happen only when there's the combination of arrows, dots and infinite.

posted by felipenmoura almost 3 years ago

Fund this Issue

$0.00
Funded
Only logged in users can fund an issue

Pull requests

Recent activities

piotr-s-brainhub cancelled funding 5.00 for  brainhubeu/ react-carousel#172
over 4 years ago
piotr-s-brainhub funded 5.00 for brainhubeu/react-carousel# 172
almost 5 years ago