Skip to content

Fix animation bug #763

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

Merged
merged 2 commits into from
May 5, 2021
Merged

Fix animation bug #763

merged 2 commits into from
May 5, 2021

Conversation

chiuam
Copy link

@chiuam chiuam commented May 3, 2021

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

I'm working on a FURN control microsoft/fluentui-react-native#669 that uses the animated library, and there's a bug where animation doesn't loop when the native driver is enabled. There's already a fix facebook#29585 for the bug but it hasn't been merged to upstream. I want to bring in this fix into our fork to unblock my work.

Changelog

[Apple] [Bug] - bug fix for Animated

Test Plan

Tested the animation logic in RNTester, I was able to repro the bug and verified that the change fixes the bug.

@chiuam chiuam requested a review from alloy as a code owner May 3, 2021 21:49
@pull-bot
Copy link

pull-bot commented May 3, 2021

Messages
📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • macOS
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against a3b5b17

@harrieshin
Copy link

don't forget that you might need to merge these changes in .63 and .62 branches

@harrieshin
Copy link

should there be a test case in the tester app?

@Saadnajmi
Copy link
Collaborator

Maybe mark with the TODO markers since it is a difference from upstream currently? And @harrieshin I think adding a test case will cause more react-native core/macOS divergence

@chiuam
Copy link
Author

chiuam commented May 4, 2021

should there be a test case in the tester app?

Actually we already have a test case for looping animation in the Native Animated Example page, the bug didn't surface there probably because the shimmer animation uses a hook instead.

@harrieshin
Copy link

should there be a test case in the tester app?

Actually we already have a test case for looping animation in the Native Animated Example page, the bug didn't surface there probably because the shimmer animation uses a hook instead.

should we extend the test case so that it has an example that uses the hook then?

@chiuam
Copy link
Author

chiuam commented May 4, 2021

should there be a test case in the tester app?

Actually we already have a test case for looping animation in the Native Animated Example page, the bug didn't surface there probably because the shimmer animation uses a hook instead.

should we extend the test case so that it has an example that uses the hook then?

I think that'd be helpful, but the scope might be bigger than I thought so let's do a followup on it.

@chiuam chiuam merged commit 85dac63 into microsoft:master May 5, 2021
chiuam added a commit to chiuam/react-native-macos that referenced this pull request May 5, 2021
@chiuam chiuam mentioned this pull request May 5, 2021
4 tasks
chiuam added a commit to chiuam/react-native-macos that referenced this pull request May 5, 2021
@chiuam chiuam mentioned this pull request May 5, 2021
4 tasks
HeyImChris pushed a commit that referenced this pull request May 5, 2021
HeyImChris pushed a commit that referenced this pull request May 6, 2021
* Fix animation bug  (#763)

* pod install

* fix pod errors

* fix pod errors

* fix pod errors
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.

4 participants