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

Carousel advances once extra after swipe, with autoplay = false #621

Closed
finnmerlett opened this issue Jul 12, 2021 · 13 comments
Closed

Carousel advances once extra after swipe, with autoplay = false #621

finnmerlett opened this issue Jul 12, 2021 · 13 comments
Labels

Comments

@finnmerlett
Copy link

finnmerlett commented Jul 12, 2021

This is a known issue with a pull request and a closed bug report, but since the issue still appears to be present in the latest version of this package on npm, I thought it could do with a new open bug report highlighting it.

Bug summary: when you install the latest version and create a carousel with autoplay={false}, then swipe either direction, after about 3 seconds the carousel will advance one slide of its own accord, as long as it isn't already on the last slide of course.

The issue of these 'ghost swipes' appeared to be related to autoplay being initialised after a swipe, regardless of the state of the autoplay prop, and the PR looked like it remedied the issue. However, the issue persists as of version 3.2.19. It is worth noting that a fork by SUCHiDEV of react-responsive-carousel and published as npm package react-responsive-carousel-2 appears to have the issue fixed. That is the package I am currently using, but I would like to get back to using the original package if possible.

EDIT: I am not using the react-responsive-carousel-2 package, because it has other bugs which have since been fixed in the main version, and instead I am using the temporary fix suggested by jonaschan, which is to set the [autoplay] interval prop to a very large number of ms).

@yeungalan0
Copy link

yeungalan0 commented Jul 15, 2021

I was able to reproduce this minimally here with version 3.2.19: https://codesandbox.io/s/brave-dew-u3vxg (all you need to do is click on a slide to have it autoPlay to the next one even though that setting is set to false)

I downloaded the current code from the Master branch, and built the demo, but could not reproduce it. Will keep digging when time allows.

@yeungalan0
Copy link

Interesting...looking at the transpiled Javascript of the u3vxg.csb.app/node_modules/react-responsive-carousel/lib/js/components/Carousel.js file (which I got from the preview in the codesandbox) I see the below:

    _defineProperty(_assertThisInitialized(_this), "onSwipeEnd", function (event) {
      _this.setState({
        swiping: false,
        cancelClick: false,
        swipeMovementStarted: false
      });

      _this.props.onSwipeEnd(event);

      _this.autoPlay();
    });

It looks like the change referenced by @finnmerlett that was supposed to fix this bug didn't come through somehow...

@yeungalan0
Copy link

yeungalan0 commented Jul 15, 2021

Also if I look at the imported module code, at node_modules/react-responsive-carousel/lib/js/components/Carousel/index.js the code looks as expected:

    _defineProperty(_assertThisInitialized(_this), "onSwipeEnd", function (event) {
      _this.setState({
        swiping: false,
        cancelClick: false,
        swipeMovementStarted: false
      });

      _this.props.onSwipeEnd(event);

      if (_this.state.autoPlay) {
        _this.autoPlay();
      }
    });

However under this path it does not: node_modules/react-responsive-carousel/lib/js/components/Carousel.js (seems like this is what makes it to the front-end)

    _defineProperty(_assertThisInitialized(_this), "onSwipeEnd", function (event) {
      _this.setState({
        swiping: false,
        cancelClick: false,
        swipeMovementStarted: false
      });

      _this.props.onSwipeEnd(event);

      _this.autoPlay();
    });

@finnmerlett
Copy link
Author

finnmerlett commented Jul 16, 2021

Yes this matches with what I experienced. I ended up pulling the master branch, building it myself and then used that as the package for now, and there was no autoplay ghost swipes at all. Weird that even in the npm module the offending code was missing in Carousel.js and there in Carousel/index.js - perhaps a build error occurred?

@milessorce
Copy link

I'm also experiencing this issue. As a temporary workaround, I am setting interval to 9999999.

@yeungalan0
Copy link

yeungalan0 commented Jul 18, 2021

@finnmerlett are you able to share how you built the module/used it? Was building it just using yarn run build? I see the built library afterwards without the issue, but not quite sure how to use it locally.

@finnmerlett
Copy link
Author

finnmerlett commented Jul 19, 2021

Yeah for sure - I looked at the node modules react-responsive-carousel folder of a project of mine that was using this package, and saw that it just had the lib folder in it as well as some of the files from the root of the project. I guessed the command was lib:build out of the available npm script commands in package.json, and when I ran it it did exactly what I'd hoped - it regenerated the lib folder with updated files.

You can then delete all of the root files that aren't found in the node_modules responsive-react-carousel folder, and you can then use the remaining files and lib folder as your module.

If you want, you can use or clone my fork of the project that I made for the purpose of having an easy install route. The build result is in the build branch. To use it direct in your project, you can just run

npm install github:finnmerlett/react-responsive-carousel#build

and it will install it in the project you're in, and record it in the package.json, just like a normal npm module.

I have made some other changes to the package, a couple of bug fixes and some augmentations that allow you to intercept or change the swipe coordinates as they are detected by the carousel, but none of them should be breaking changes and you should be able to use the carousel just like the original package.

I haven't documented my changes, at some point I will do and will maybe make a PR to get them merged in. For now, if you are interested, just check out the commit history.

@yeungalan0
Copy link

Awesome! Thanks for the tips @finnmerlett, I'll try them out and see how things look! Hopefully the next release will fix this weird build issue, or maybe someone who can push out a new release will see this soon... 🤞

@its-nate
Copy link
Contributor

@finnmerlett my fix for this was just merged into the latest release, 3.2.21, so you can come back to the original package if you like and can close this issue 👍

@charleskoehl
Copy link

charleskoehl commented Aug 21, 2021

@its-nate , thanks very much for this fix, but it's still happening on safari iphone 11 ios 14.7.1 with v.3.2.21

slides.length === 5

image

      <Carousel
        autoPlay={false}
        showArrows
        showIndicators={false}
        showThumbs={false}
        showStatus={false}
        swipeable
      >
        {slides}
      </Carousel>

@its-nate
Copy link
Contributor

Hi @charleskoehl that's weird... You're right though, I just whipped up a Codesandbox with 3.2.21 and the problem persists. However, my fork that I built the changes on (with just that one change made from PR #624) continues to work as expected. You can run a diff if you want, I promise the changes from that PR linked above are the only changes I made from the original repo. If you want, maybe you can try installing from my fork's build branch to confirm if it works for you?

"react-responsive-carousel": "github:its-nate/react-responsive-carousel#build"

One note, I noticed that when I'm using my own fork in my project, infiniteLoop, when set to true, does not properly wrap to the first slide; it typewriter-style carriage returns to the first slide. This isn't a problem for my own use-case but may be for yours.

If it works for you and you're interested, I could officially publish my fork as a branch as there seems to be something different happening in the official package build process that seems to be affecting this issue.

@renancleyson-dev
Copy link
Contributor

renancleyson-dev commented Jan 11, 2022

Hi everyone, I stopped contributing to this repo because of time (and I wanted to work on some other open-source projects too). TBH, I don't know why this comment were ignored(the bug was already fixed locally since my PR, and there was still some PRs trying to fix it), but it's important to point out the problem with this repo. @leandrowd had done a great job by sparing his time to maintain this library, but still, though, the root cause of this bug was PR #511, which probably wasn't reviewed as we could avoid all of this with simple manual tests and some code review. I'm sorry for not helping on this before too, I just ignored GH notifications at the time and I could be useful.

Anyway, I'm not sure if the bug persists but when I find some time I will try to see what I can do, but I don't think the maintainer should maintain it if he doesn't have much time to review the PRs, as poor reviews or not reviewing PRs at all and still merging it will only make the code more buggy even if he only wants to help.

@stale
Copy link

stale bot commented Jul 10, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 10, 2022
@stale stale bot closed this as completed Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants