-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
upgrade to lottie-android 3.0.0 #485
Conversation
…instead this property is auto-detected. See: airbnb/lottie-android#1048
Original change to deprecate CacheStrategy is here: airbnb/lottie-android@3f9ee24 That commit describes the motivations and nature of the change. This commit did the actual removal of the deprecated enum: airbnb/lottie-android@e76e445
…ow, so in this case we'll try a hash of the json string itself.
compile "com.facebook.react:react-native:+" // From node_modules | ||
|
||
implementation project(':lottie-react-native') | ||
implementation 'androidx.legacy:legacy-support-v4:1.0.0' |
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.
Great idea with the update! But I have some reservations about updating RN libraries to AndroidX
.
I personally would love if everyone would switch to androidx all at once so we could avoid fragmentation as much as possible., but it's just not feasible. Right now 99% of RN libraries do not support android jetpack so adding dependency with androidx import is just gonna break about every app.
Can't help but notice that lottie-react-native
uses Support-v4
only for ViewCompat.isAttachedToWindow()
in line https://github.com/react-native-community/lottie-react-native/blob/master/src/android/src/main/java/com/airbnb/android/react/lottie/LottieAnimationViewManager.java#L118 and https://github.com/react-native-community/lottie-react-native/blob/master/src/android/src/main/java/com/airbnb/android/react/lottie/LottieAnimationViewManager.java#L144
I propose to replace this call with https://developer.android.com/reference/android/view/View.html#isAttachedToWindow(), so it's gonna be view.isAttachedToWindow
, and removing support-v4
dependency altogether. Only downside to this is loosing compatibility with android apis 16-19 (so about 3.2% of Android devices https://developer.android.com/about/dashboards) but you gain compatibility with AndroidX and Android Support Library projects, and thats huge!
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.
Anychance of this, getting merged without the change in SDK? It seriously is causing issues.
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.
@rozPierog I'm not sure if I get your suggestion. Could you send a PR with it so I can give it a try? Thanks!
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.
@influx6 what kind of issues current lottie-react-native
is causing at the moment? As I mentioned, I'm a bit hesitant to go ahead with this PR as per @rozPierog comment
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 also won't advise it due to androidx support libraries. We resolved our issues, as we realized we were exported in bodymovin with lottie3 format
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.
@influx6 Your comment was incredibly helpful. Almost all the animations on the LottieFiles Community site were working but even the simplest scaling animations of mine weren't working. Turns out it's the setting you mentioned under Advanced Settings. Same solution as to @airbnb/lottie-web#1561 incase anyone else stumbles upon this. It looks like the documentation has been updated to say the current version requires AndroidX? This was confusing.
Any chances to get this merged? I need this PR to fix my issue: |
as @rozPierog mentioned, migrating to androidX could cause issues with a lot of other libraries. I'm reluctant to go ahead with this PR at this moment |
@emilioicai take a look at this: https://github.com/mikehardy/jetifier#module-maintainers and this: react-native-community/discussions-and-proposals#129 |
@emilioicai |
Now that RN 0.60 is out, it would make sense to merge and release 3.0.0 -branch of lottie-react-native that would bring androidx support to the module and have 2.6.x branch exist while waiting for the community to adopt androidx more. |
As of RN 0.60, according to the release blog post
|
Any news on this? |
AndroidX support is already present in release 3.0.4. I'm working on this but it has to be done in a separate PR as build.gradle has changed too much. I will keep this PR open until the other is on its way as a reference. |
closing in favor of #527 |
@emilioicai okay thanks! |
No description provided.