Skip to content

Conversation

@GabrielCartier
Copy link

Heavily based on @Satyam-code143 version, also added a way to clear the timeout to avoid leaks.

Comment on lines +98 to +105
componentDidMount() {
const { autoplay, interval } = this.props;
if (autoplay) {
this.state.intervalId = setInterval(() => {
this.moveSlide(1);
}, interval);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion it might be better to make autplay/interval prop changes affect the interval even after the component was mounted.

Comment on lines +123 to +125
if(this.state.intervalId) {
clearInterval(this.state.intervalId);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(this.state.intervalId) {
clearInterval(this.state.intervalId);
}
if (this.state.intervalId) {
clearInterval(this.state.intervalId);
}

Comment on lines +13 to +14
autoplay: true,
interval: 2,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can just use interval here and default it to null or 0, this would simplify the API and prevent users from accidentally setting inconsistent prop values.

{
"name": "react-spring-3d-carousel",
"version": "1.2.1",
"version": "1.3.1",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"version": "1.3.1",
"version": "1.3.0",

@hebrewtimeshepherd
Copy link
Owner

hebrewtimeshepherd commented Jul 1, 2022

Looks pretty good overall, left some minor comments and would be happy to merge once they're addressed. Great work! 🎉

@GabrielCartier
Copy link
Author

Don't really have much time to spend on this, I had done this PR when I needed the lib for a specific project.

Will try to see if I can do the suggested changes, else, if anyone want to pick it up, feel free to :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants