brainhubeu / react-carousel

A pure extendable React carousel, powered by Brainhub (craftsmen who ❤️ JS)
https://brainhub.eu/
MIT License
1.07k stars 164 forks source link

init too much list items on the infinite mode #497

Closed sinchang closed 4 years ago

sinchang commented 4 years ago

Describe the bug if we have 3 items originally will clone 18 items on the infinite mode

To Reproduce go to https://codesandbox.io/s/still-architecture-j2e9m

open console and run document.querySelectorAll('.BrainhubCarouselItem')

Expected behavior reduce the number of initial list items

Screenshots If applicable, add screenshots to help explain your problem.

Environment

Additional context Finally. I found that we have the config numberOfInfiniteClones (current is 3) to control the number of clones.

piotr-s-brainhub commented 4 years ago

@sinchang

Thanks for reporting this.

I reproduced that. It happens also in the docs/ demo. It's strange that there are so many slides even before switching slides and it's contrary to the numberOfInfiniteClones.

You're welcome to open a PR.

You can also invite your colleagues to like (👍) this issue, so more 👍it has, it's more likely to be fixed by us.

piotr-s-brainhub commented 4 years ago
Screenshot 2020-05-21 at 12 16 40
sinchang commented 4 years ago

@Lukasz-pluszczewski can we modify the numberOfInfiniteClones to 1

Lukasz-pluszczewski commented 4 years ago

The reason it has been set to 3 is to allow controlled carousel to show smooth animation when changing the slide index by a large number (e.g. if you have 3 slides, and want to animate infinite carousel moving by 10 slides to the right)

It may have a significant performance impact especially on mobile, so I'm ok with changing the default value to 1, but keeping in mind the above, I would maybe add it as optional parameter.

R1cro commented 4 years ago

@piotr-s-brainhub @Lukasz-pluszczewski is there any way to change numberOfInfiniteClones to 1 through props?

I would be grateful if you would suggest how this can be regulated.

Thank you.

CC: @sinchang

piotr-s-brainhub commented 4 years ago

@R1cro

Currently, it's not possible because it's hardcoded in the config but a simple PR should be able to change this.

RobertHebel commented 4 years ago

@R1cro it's possible in carousel v2 (currently in beta). More details here https://brainhubeu.github.io/react-carousel/docs/plugins/plugins#infinite-plugin

jefferymills commented 4 years ago

@R1cro

Currently, it's not possible because it's hardcoded in the config but a simple PR should be able to change this.

Would a PR making this changeable through props get accepted for release before 2.0? We currently experience crashes on some IOS devices and in our case we don't need that many clones. I would be happy to add it if it would get merged. If not then we can work around it until 2.0 is released.