-
Notifications
You must be signed in to change notification settings - Fork 333
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
Lottie support, but very raw state #134
Conversation
…ementLottie class
Thanks for this! Will need some changes before we merge into master. But I can take over from here. Some changes I intend to to before merging (in addition to your list):
|
I see this PR tried to add a new element. Sure it is a good idea? wouldnt something like a custom decorator make more sense? or even: |
There will be no changes to the core library, as I intend to move that element out and into the sample as a plugin, so it won't do anything unless you manually copy over the plugin. Does that ease your concern? :) Otherwise, I'm willing to discuss everything before I merge it to master. |
By the way, @DiamondHat, do you have a source and a license for the lottie animation you included? |
I do not really have a preference where integration lives. Just in what form it manifests. But maybe there is no cause for concern. We have |
I think it makes more sense as an element, rather than a decorator, especially since it has intrinsic size. Plus, I think this is the easiest way to update it regularly. |
@mikke89 I used json animations from rlottie repo, and guessed it's all by MIT license. I don't think they are used commercial animations to publish them by MIT license xD But you can check this file https://github.com/Samsung/rlottie/blob/master/COPYING |
The lottie element has been refactored into a separate sample, injected as a plugin. Co-authored-by: Michael R. P. Ragazzon <mikke89@users.noreply.github.com>
Need to be fixed:
Done:
I am done here, and can't support further (maybe I can add fixes for color interpretation sometime though).