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

upgrade to lottie-android 3.0.0 #485

Closed
wants to merge 9 commits into from

Conversation

emilioicai
Copy link
Member

No description provided.

compile "com.facebook.react:react-native:+" // From node_modules

implementation project(':lottie-react-native')
implementation 'androidx.legacy:legacy-support-v4:1.0.0'

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!

Copy link

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.

Copy link
Member Author

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!

Copy link
Member Author

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

Copy link

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

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.

@changLiuUNSW
Copy link

Any chances to get this merged? I need this PR to fix my issue:
airbnb/lottie-android#1252

@emilioicai
Copy link
Member Author

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

@rozPierog
Copy link

rozPierog commented Jun 27, 2019

@changLiuUNSW
Copy link

@emilioicai
https://twitter.com/gpeal8/status/1132824596749537280
It looks like they have an one-off 3.0.3-support artifact that supports non-androidx apps

@plouh
Copy link

plouh commented Jul 9, 2019

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.

@Javi
Copy link

Javi commented Jul 18, 2019

As of RN 0.60, according to the release blog post

With this change, React Native apps will need to begin using AndroidX themselves.

@zwenza
Copy link

zwenza commented Jul 24, 2019

Any news on this?

@emilioicai
Copy link
Member Author

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.

@emilioicai
Copy link
Member Author

closing in favor of #527

@emilioicai emilioicai closed this Jul 25, 2019
@zwenza
Copy link

zwenza commented Jul 25, 2019

@emilioicai okay thanks!
My problem with 3.0.4 is airbnb/lottie-android#1252 like @changLiuUNSW
mentioned. So I have no issues with AndroidX but with this particular error that seems to be fixed in lottie-android 3

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

8 participants