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
fix: carousel should stop immediately after interval is set to false #2791
base: master
Are you sure you want to change the base?
fix: carousel should stop immediately after interval is set to false #2791
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if you can rewrite the test to use react testing library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better solution as mentioned in the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&& (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. Sure, sounds good to me.
bf99409
to
ca383f0
Compare
Closes #2779