Skip to content
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

resolve #116 #117

Merged
merged 3 commits into from
Dec 23, 2018
Merged

resolve #116 #117

merged 3 commits into from
Dec 23, 2018

Conversation

lluisrojass
Copy link
Contributor

Resolves #116

componentWillReceiveProps will now reset frames to default state upon receiving an array of frames differing from state frames.

@amio
Copy link
Owner

amio commented Dec 5, 2018

Since componentWillReceiveProps has been marked as deprecated, I think we may better handle this in componentDidUpdate or getDerivedStateFromProps.

…teFromProps with added componentDidUpdate functionality
Copy link
Owner

@amio amio left a comment

Choose a reason for hiding this comment

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

Thanks @lluisrojass , just a few refinements

src/carousel.js Outdated
if (this.state.frames.length && this.state.frames.length !== prevState.frames.length) {
// reset to default state
this.hideFrames()
translateXY(this.refs['f0'], 0, 0, 0)
Copy link
Owner

Choose a reason for hiding this comment

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

Should use this.prepareAutoSlide() instead of translateXY(this.refs['f0'], 0, 0, 0) to get all frames positioned correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amio just noticed the comments :/ will be able to get to it within 24 hours, thanks for the patience and I apologize for the latency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amio this has been addressed

src/carousel.js Outdated
if (frames.length && frames.length !== prevState.frames.length) {
nextState.current = 0
}

Copy link
Owner

Choose a reason for hiding this comment

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

Extra spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amio this has been addressed in latest commit.

@amio amio merged commit de5d598 into amio:master Dec 23, 2018
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