akiran / react-slick

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

afterChange is not triggered when adaptiveHeight is true #1262

Open Drafter500 opened 6 years ago

Drafter500 commented 6 years ago

https://codesandbox.io/s/k6n066ox5

yunyong commented 6 years ago

So I changed it to "beforeChange()" :(

clairefritz commented 6 years ago

Same issue here!

jeremiah-s-eldridge commented 6 years ago

I saw this before adding adaptiveHeight = true. My slides can have variable heights, and if the height of the img element changes the afterChange event fails to dispatch. If you add adaptiveHeight=true, it gets exponentially worse.

jeremiah-s-eldridge commented 6 years ago

it looks like the animationEndCallback is never executed under these conditions - haven't found where it should be executed from yet

martinek commented 6 years ago

So I figured out why this happen in our case.

We use beforeChange and afterChange to disable clicking on items while swiping. In our case, when you swipe, sometimes beforeChange would be called but afterChange would not be called. This would stuck wrapper in "sliding" state with disabled clicks.

What happens in library is that when slide is moved, slider height is changed to accomodate for higher/shorter slide height, this triggers window resize (in our case) and resizeWindow callback which clears this.animationEndCallback which causes afterChange to not fire at all.

I'm not sure how to fix this as I'm not sure why animationEndCallback is being reset. There is some comment about autoplay, not sure how that work.

janhesters commented 6 years ago

Oh wow I struggled with this for about an hour before I stumbled upon this thread. After reading this, let me share with you the workaround I used:

const speed: number = 500;
const settings: Settings = {
  adaptiveHeight: true,
  // NOTE: afterChange is broken when adaptiveHeight is set to true. See:
  // https://github.com/akiran/react-slick/issues/1262. Therefoe this hacky solution.
  beforeChange: (current, next) =>
    setTimeout(() => this.setState(prevState => ({ ...prevState, currentSlide: next })), speed),
  speed
};
epignosisx commented 6 years ago

I debugged the issue a little bit and the problem is that the callback to this timeout:

https://github.com/akiran/react-slick/blob/93c37bd42f5d91e0edfcb83be4e1f8a028ee8e56/src/inner-slider.js#L397

is not executing because during the wait period resizeWindow is being called and it clears the timeout:

https://github.com/akiran/react-slick/blob/93c37bd42f5d91e0edfcb83be4e1f8a028ee8e56/src/inner-slider.js#L213-L214

The resizeWindow method is triggered by the resizeObserver:

https://github.com/akiran/react-slick/blob/93c37bd42f5d91e0edfcb83be4e1f8a028ee8e56/src/inner-slider.js#L84

tchamblee commented 5 years ago

Here's what worked for me, just include this in your settings object:

const speed = 500
const settings = {
  beforeChange: (current, next) => {
    setTimeout(afterChange(next), speed)
  },
  speed
}
sjovall commented 5 years ago

Damn... took me a while to figure this out.

Is anyone working on a proper solution to this issue?

Btw, this only seems to be a problem in cases where custom buttons are used for the navigation that utilizes the next / prev methods. The default buttons provided by the slider works fine even if adaptiveHeight is being used.

Here's my own sandbox if anyone is interested https://codesandbox.io/s/mykn4lq78j.

alex-lechner commented 5 years ago

Any update on this issue? The workaround does not work for me.

alex-lechner commented 5 years ago

So I've found a workaround now. The slider in my application needs to keep track of the slider's dragging state because whether the user swipes or not depends on if the item in the slider is clickable. The items are only clickable if the user does not swipe. Here's my code:

constructor(props, context) {
    super(props, context);
    // set states in constructor
    this.state = {
      dragging: false,
      hasAdaptiveHeight: true
    };
}

render() {
    const settings = {
      adaptiveHeight: this.state.hasAdaptiveHeight,
      beforeChange: (current, next) => {
        this.setState({ dragging: true }, () => {
          if (this.state.hasAdaptiveHeight) {
            setTimeout(() => {
              this.setState({ dragging: false });
            }, 1000);
          }
        });
      },
      afterChange: next => {
        // this is only called if adaptiveHeight is set to false
        this.setState({ dragging: false });
      }
    }

    return (
      <Slider {...settings}>
        // your items here
      </Slider>
    );
}
PotLid commented 5 years ago

Oh wow I struggled with this for about an hour before I stumbled upon this thread. After reading this, let me share with you the workaround I used:

const speed: number = 500;
const settings: Settings = {
  adaptiveHeight: true,
  // NOTE: afterChange is broken when adaptiveHeight is set to true. See:
  // https://github.com/akiran/react-slick/issues/1262. Therefoe this hacky solution.
  beforeChange: (current, next) =>
    setTimeout(() => this.setState(prevState => ({ ...prevState, currentSlide: next })), speed),
  speed
};

This work arounds perfectly works on me, thanks :)

GuyPaddock commented 3 years ago

We just ran into this, especially on Safari 13. In our case, we're not using adaptiveHeight but changes in the slider coincide with changes in flow elsewhere on the page (e.g. we're using the slider as a timeline component, below which other content appears based on what slide of the timeline is selected). afterChange does not get called consistently, and adding debug print statements to react-slider, I can confirm that clearTimeout(_this.animationEndCallback) being called from windowResize is happening precisely at the times that the afterChange callback is being dropped.

So, perhaps a better title for this issue should be that "afterChange does not get called if slider animation causes page dimensions to change".

fabianrichter commented 9 months ago

Oh wow I struggled with this for about an hour before I stumbled upon this thread. After reading this, let me share with you the workaround I used:

const speed: number = 500;
const settings: Settings = {
  adaptiveHeight: true,
  // NOTE: afterChange is broken when adaptiveHeight is set to true. See:
  // https://github.com/akiran/react-slick/issues/1262. Therefoe this hacky solution.
  beforeChange: (current, next) =>
    setTimeout(() => this.setState(prevState => ({ ...prevState, currentSlide: next })), speed),
  speed
};

This does not completely solve the issue as the transition is not working with this approach. Are there any updates on this issue?