-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Merge ViewDescriptors implementation #6123
Conversation
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.
LGTM
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.
Just blocking it for now because I know this code more in-depth and want to review it properly before we merge it. 🙏
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.
We need some TS fixes for these changes in the pipeline, but it's of the least concern for now. Beside this, all is solid!
Summary
The motivation behind this PR was the lack of animation in the web implementation when React.StrictMode was enabled. While debugging, I discovered we had two similar mechanisms for storing connection between components and animations: one for mobile animations and one for web animations. I realized that the native implementation worked flawlessly without any issues on the web as well, as it had been recently refactored to align with React's guidelines. I decided to remove the flawed web implementation and replace it with mobile implementation.
Test plan
<React.StrictMode></React.StrictMode>
and see if animations works.