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

Add support for calling .start() multiple times on animated sequence #26791

Closed
wants to merge 2 commits into from

Conversation

lbrdar
Copy link

@lbrdar lbrdar commented Oct 9, 2019

Summary

This PR solves #26790

Changelog

[General] [Fixed] - Fix error thrown when sequence animation was started for the second time

Test Plan

I've added a test to cover this case

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@TheSavior
Copy link
Member

What happens if start is called again before the first animation sequence finishes?

@react-native-bot react-native-bot added the No CLA Authors need to sign the CLA before a PR can be reviewed. label Oct 10, 2019
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 10, 2019
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@react-native-bot react-native-bot removed the No CLA Authors need to sign the CLA before a PR can be reviewed. label Oct 10, 2019
@lbrdar
Copy link
Author

lbrdar commented Oct 10, 2019

@TheSavior good question. It won't break but the animation won't play properly either.

I don't think other types of animation (e.g. parallel) handle that case. I guess the user should take care of that? What do you expect to happen in that case?

@TheSavior
Copy link
Member

I'm not sure what the "correct" behavior is here, I just wanted to flag it as it seems like a common edge case of being able to call start multiple times.

Can parallel be called multiple times? What happens for parallel? Naively, I'd hope that they'd be consistent with respect to each other.

@cpojer
Copy link
Contributor

cpojer commented Oct 14, 2019

I was going to bring up the same concern that @eliwhite did. I think it makes more sense to me to throw an error if start is called more than once. What do you think?

@lbrdar
Copy link
Author

lbrdar commented Oct 25, 2019

@cpojer are you suggesting that RN always throw an error when start() is called again or just if it's called before the previous iteration is complete?

I'd say it makes sense to throw if called while the previous iteration is still running. Otherwise no. Because I don't see a reason to block a user from starting the same animation multiple times.

@cpojer
Copy link
Contributor

cpojer commented Oct 28, 2019

Yeah I think that makes sense.

@lbrdar
Copy link
Author

lbrdar commented Dec 18, 2019

@cpojer @TheSavior updated per your request.

Could you help me with failing checks here?

@cpojer
Copy link
Contributor

cpojer commented Jan 7, 2020

I took an extended break, sorry I didn't get back to your PR earlier. I thought about this some more and do not feel comfortable changing the behavior right now. While I do not know what the original authors intended, I recommend creating new instances for new animations instead of re-using the same ones.

@cpojer cpojer closed this Jan 7, 2020
facebook-github-bot pushed a commit that referenced this pull request May 23, 2023
Summary:
Previously, we'd call and use getSnapshot on the second render resulting
in `Warning: Text content did not match. Server: "Nay!" Client: "Yay!"`
and then `Error: Text content does not match server-rendered HTML.`.

Fixes #26095. Closes #26113. Closes #25650.

---------

DiffTrain build for commit facebook/react@4cd7065.

Reviewed By: mofeiZ

Differential Revision: D45829972

Pulled By: sammy-SC

fbshipit-source-id: e89404c53a1b8478d22252ca24ddcc9647744d37

Co-authored-by: eps1lon <silbermann.sebastian@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Animated Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants