akiran / react-slick

React carousel component
http://react-slick.neostack.com/
MIT License
11.72k stars 2.1k forks source link

[BUG] slickNext, slickPrev and slickGoTo broken when focusOnSelect={true} and fade={false} #1949

Open felixmeziere opened 3 years ago

felixmeziere commented 3 years ago

The solution to this issue https://github.com/akiran/react-slick/issues/1074 which was solved in this commit https://github.com/NickIannelli/react-slick/commit/30d5c2fe916c26db63caeef2d53caeee11d03e83 seems to now break the three methods slickNext, slickPrev and slickGoTo.

Actual behaviour: when calling slickNext on the first slide, the slider immediately jumps to the 2nd slide and then slides to the third slide. Expected behaviour: just slide to the second slide.

This only happens if fade={false} and focusOnSelect={true}.

Fix is to remove the setTimeouts on the methods (seems like a weird solution anyway to be honest, that doesn't seem like a great way to solve race condition :/).

I'm not sure on how to solve this if the "use slider in componentDidMount" usecase is important (shouldn't we use the initialSlide prop instead? Or a setTimeout particular to the application that waits a little bit before navigating? I suggest removing the setTimeouts in the code here...).

I don't have time for a PR but if it's ok to not handle the usecase above then in the library the setimeouts here should be removed:

slickPrev = () => {
    // this and fellow methods are wrapped in setTimeout
    // to make sure initialize setState has happened before
    // any of such methods are called
    this.callbackTimers.push(
      setTimeout(() => this.changeSlide({ message: "previous" }), 0)
    );
  };
  slickNext = () => {
    this.callbackTimers.push(
      setTimeout(() => this.changeSlide({ message: "next" }), 0)
    );
  };
  slickGoTo = (slide, dontAnimate = false) => {
    slide = Number(slide);
    if (isNaN(slide)) return "";
    this.callbackTimers.push(
      setTimeout(
        () =>
          this.changeSlide(
            {
              message: "index",
              index: slide,
              currentSlide: this.state.currentSlide
            },
            dontAnimate
          ),
        0
      )
    );
  };

Sandbox : https://codesandbox.io/s/react-slick-playground-forked-cdbf8?file=/index.js

Otherwise it's a problem with focusOnSelect, no time to investigate more now sorry :/

thinkanddoit commented 3 years ago

Can i try this?

VadimSvirdoff commented 3 years ago

@felixmeziere could you share example with bug in https://codesandbox.io In this example all work right https://react-slick.neostack.com/docs/example/previous-next-methods

felixmeziere commented 3 years ago

@VadimSvirdoff I put a sandbox above with the bug reproduced, what is missing from it to be useful? :-)