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

Fix frame setting issue for CompatibleAnimationView. #1878

Merged
merged 5 commits into from
Dec 28, 2022

Conversation

snow-google
Copy link
Contributor

As discussed on the GitHub page in the past, CompatibleAnimationView does not correctly set up the constaints as to make the underlying AnimationView fill the entirity of the frame provided to CompatibleAnimationView. #1091

This fix is the same that was discussed in the issue thread and resolves the frame setting issue.

As discussed on the GitHub page in the past, CompatibleAnimationView does not correctly set up the constaints as to make the underlying AnimationView fill the entirity of the frame provided to CompatibleAnimationView.
airbnb#1091

This fix is the same that was discussed in the issue thread and resolves the frame setting issue.
@calda
Copy link
Member

calda commented Dec 24, 2022

I don't think this change is backwards-compatible, it seems like it could easily be a breaking change for consumers who have code that relies on translatesAutoresizingMaskIntoConstraints defaulting to false. Could you share more details about your issue? Why does this fix the problem? Is there another solution we could use that would have less effect on existing consumers?

@snow-google
Copy link
Contributor Author

As it is, the compatible view is a wrapper on the underlying animation view. When the compatible view has its frame set, the underlying animation view does not get these updates. This is not a problem when using the aniamtion view directly which means that it is only ObjC compatible views that have this unexpected behavior. In essense, the abstraction and encapsulation that the compatible view is supposed to create are broken by the current behavior which requires compatible view users to investigate how to resolve the issue. It is possible that the workaround previously discussed on this forum makes it less likely for people to report this issue, but for my use case, overriding the value in our source code is incredibly challenging.

Copy link
Member

@calda calda left a comment

Choose a reason for hiding this comment

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

Ok, thanks for the explanation. I think this change makes sense considering translatesAutoresizingMaskIntoConstraints also defaults to true for LottieAnimationView -- seems like we'd want both to have the same behavior.

I went ahead and also added a test case for CompatibleAnimationView to the snapshot test suite.

@calda calda enabled auto-merge (squash) December 27, 2022 23:59
@calda calda merged commit 85630ac into airbnb:master Dec 28, 2022
@snow-google
Copy link
Contributor Author

Thank you so much for the work to accept and verify this change! I appreciate the responsiveness of the team!

iago849 pushed a commit to atteamapps/lottie-ios that referenced this pull request Feb 8, 2024
…`CompatibleAnimationView` (airbnb#1878)

Co-authored-by: Cal Stephens <cal@calstephens.tech>
MoroziOS pushed a commit to MoroziOS/tmg-lottie-ios that referenced this pull request May 22, 2024
…`CompatibleAnimationView` (airbnb#1878)

Co-authored-by: Cal Stephens <cal@calstephens.tech>
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.

None yet

2 participants