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

[Compose] Add merge paths flag #1744

Merged
merged 7 commits into from
Feb 24, 2021
Merged

Conversation

jossiwolf
Copy link
Contributor

In relation to #1734

LottieAnimationView had merge paths disabled by default, but LottieAnimation enables them by default. As long as merge paths contain known bugs, I think it makes sense not to change that behavior - what do you think?
I added a flag for it to the state. If you want, we can add it as a constructor param too, but I feel like that's not needed as it's not that commonly used.

Cheers

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

This flag is set to false by default to align with LottieAnimationView.

Added some documentation too
@jossiwolf
Copy link
Contributor Author

@gpeal I added a toggle for merge paths to the Compose sample. Let me know if you want to have that as part of this PR.

lottie-compose-merge-paths-toggle-sample.mp4

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

Pretty close! Just a couple small comments.

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

Thanks!

@gpeal gpeal merged commit 4b69bc1 into airbnb:master Feb 24, 2021
@jossiwolf jossiwolf deleted the jw/merge-paths-flag branch February 24, 2021 16:20
@jossiwolf
Copy link
Contributor Author

Awesome, thanks so much!

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.

3 participants