-
-
Notifications
You must be signed in to change notification settings - Fork 588
feat(Android, Tabs): Update approach for loading external sources for tab icons #3216
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
Conversation
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 haven't checked the runtime but it looks good!
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.
Code changes look good. Have a couple of questions / suggestions. Please answer them.
I'll test the runtime in a moment.
android/src/main/java/com/swmansion/rnscreens/gamma/tabs/TabScreenViewManager.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/gamma/tabs/TabsImageLoader.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/gamma/tabs/TabsImageLoader.kt
Outdated
Show resolved
Hide resolved
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.
Let's just make it internal
and we're looking good.
I'll test runtime in a moment & if everything is fine, we can proceed.
android/src/main/java/com/swmansion/rnscreens/gamma/tabs/image/TabsImageLoader.kt
Outdated
Show resolved
Hide resolved
…/TabsImageLoader.kt Co-authored-by: Kacper Kafara <[email protected]>
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.
Hey! Started to test the runtime & noticed that our TestBottomTabs
require update!
Can we create dedicated test / example screen (maybe that's even better) that tests images with bottom tabs?
Edit: was checked out on wrong PR, sorry for confusion
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.
Tested the runtime -- seems to work fine. Great job here.
Note
There is one thing I still don't like - and it is that we have a icon loading logic inside component view manager. I don't think this should block this PR from landing, however we should create a ticket for a small refactor (and first discussion about how it should be organised). Would you do it?
Description
In the initial approach, we used Coil image loading library. However, after the 1st release, Coil has introduced several issues that had an impact on the stability and maintainability.
After reevaluating other options, we've decided to try migrating to Fresco. This choice aligns better because React Native also relies on Fresco internally. Using Fresco should improve consistency with the RN and reduce our maintenance overhead.
One known limitation is that Fresco doesn't support loading images in SVG format. We’ll need to explore alternative strategies to adapt.
Note:
Slightly depends on: #3214
Fresco doesn't support SVGs, so we decided to handle them by passing assets via the
AndroidStudio
as vector icons - this approach simplifies a lot of things, especially thatwill cover SVGs, so we can omit adding a support here. I'll ensure to mention there that SVG should be passed with
drawableResourceAndroid
only.Changes
Test code and steps to reproduce
Tested with BottomTabs example.
Checklist