-
-
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
Runtime snapshot runtime tests of Layout Animation on fabric #6538
base: main
Are you sure you want to change the base?
Conversation
apps/common-app/src/examples/RuntimeTests/ReJest/TestRunner/UpdatesContainer.ts
Outdated
Show resolved
Hide resolved
// TODO Implement native snapshots for layout animations on Fabric | ||
_updateNativeSnapshot([{ tag, update }], jsUpdates.value.length - 1); | ||
} else { | ||
brokenNativeSnapshotsOnFabric.value = true; |
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.
I think you can safely move this assignment to the createUpdatesContainer
method.
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.
This would have some negative side effects, for example it would broke this test:
Line 46 in ddabf41
test.each([ |
Cause only in case of layout animations the native snapshots are unimplemented, so we can assume that if function notifyAboutProgress
has never been called, then native snapshots work fine on fabric.
if (_IS_FABRIC && (-1) in sortedUpdates) { | ||
return sortedUpdates[-1]; | ||
} |
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.
This is something we need to verify. Does it happen all the time or just sometimes? Additionally, it will break in cases where we want to test animations for more than one component.
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.
Yes, it happens all the time and breaks some of our animations. However most of our tests features only one component.
if ((key as string) === 'opacity' && jsValue === 1 && nativeValue === undefined) { | ||
// undefined opacity may get translated into 1, as it is the default value | ||
valuesAreMatching = true; | ||
} |
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.
Also, here we need to verify why in the JS update, we can receive undefined values
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.
In our layout animations we use function maybeRestoreOpacity
a lot. After the restoration the opacity is explicitly defined as 1, even if it had been undefined.
Summary
Snapshots of layout animations were not implemented for fabric (as layout animations were not implemented). In this PR I enable JS snapshots for fabric layout animations.
Test plan