fix: carousel should stop immediately after interval is set to false #2791
Conversation
MariusJam
force-pushed
the
fix/immediate-stop-on-false-interval
branch
from
4e6e447
to
bf99409
Compare
|
Thank you so much for the PR. A better solution, in my opinion, would be to clear the interval if the props has changed. Currently, there is another bug which is once the carousel is stopped, it does not restart even if you toggle the interval back to a number. the above fixes that issue also. (We are calling setInterval again because it has a clearInterval inside it which takes care of clearing the interval and running the interval again only if needed). Also the second thing is currently we are deprecating enzyme so it would be great if you can change the tests to run using react testing library instead. |
| @@ -765,5 +766,23 @@ describe('Carousel', () => { | |||
| jest.advanceTimersByTime(1000); | |||
| expect(next).toHaveBeenCalled(); | |||
| }); | |||
There was a problem hiding this comment.
It would be great if you can rewrite the test to use react testing library
There was a problem hiding this comment.
I'm not familiar with react testing library but I've pushed some changes. Please let me know if it's fine now.
src/Carousel.js
Outdated
| @@ -112,7 +112,12 @@ class Carousel extends React.Component { | |||
| } | |||
|
|
|||
| componentDidUpdate(prevProps, prevState) { | |||
| if (prevState.activeIndex === this.state.activeIndex) return; | |||
| if ( | |||
There was a problem hiding this comment.
better solution as mentioned in the comments
There was a problem hiding this comment.
Hi,
I can implement the solution you propose indeed but there will be one side effect (should it be considered as a breaking change?): it will restart the interval to 0 at every change. So, let's say you set an interval at 5s and, after 4s, you change the interval to 10s, it will wait 10 more seconds before transitioning. So, in total, it will be 14 seconds before the first transition whereas the current behaviour is different: it does the first transition after 5 seconds and a second transition after 10 seconds.
I guess that if we want to avoid this side effect, we should check that we are in a case of stopping/restarting the carousel. For instance:
componentDidUpdate(prevProps, prevState) {
if (prevProps.interval !== this.props.interval
&& (prevProps.interval === false || this.props.interval === false)) {
this.setInterval();
}
if (prevState.activeIndex === this.state.activeIndex) return;
this.setInterval();
}Would you be okay with this proposal?
There was a problem hiding this comment.
&& (prevProps.interval === false || this.props.interval === false)) { this will be the case every time we stop and restart right? which means this is always going to be true I guess (as in on the edge case you mentioned).
There was a problem hiding this comment.
this will be the case every time we stop and restart right?
Yes indeed and I think that's what we want. In that way we fix both issues: the one described in #2779 and the one you mentioned in your first comment.
which means this is always going to be true I guess (as in on the edge case you mentioned).
I don't think so. Or I'm missing something. 🤔. As far as I understand, prevProps.interval would be equal to 5000 and this.props.interval to 10000 in my example. So, in that case, && (prevProps.interval === false || this.props.interval === false)) { is going to be false.
There was a problem hiding this comment.
My bad. Sure, sounds good to me.
MariusJam
force-pushed
the
fix/immediate-stop-on-false-interval
branch
from
bf99409
to
ca383f0
Compare
Closes #2779