-
-
Notifications
You must be signed in to change notification settings - Fork 588
refactor(Tabs): Refactor APIs for icons #3214
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
base: main
Are you sure you want to change the base?
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 really like this.
Co-authored-by: kligarski <[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.
I kinda like the changes. Have few remarks regarding naming (will describe them in follow-up review).
We need to wait couple more days until I reach expo-router & coordinate this change (@Ubax is on PTO currently)
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'd only change following suffixes & hav one remark regarding whether the logic should be run. The rest looks really nice.
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.
Like the PR! Let's wait for getting in touch with expo though
… tab icons (#3216) ## 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 that ``` PlatformIconAndroid = | { type: 'drawableResourceAndroid'; name: string; } ``` will 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 - Removed Coil - Replaced logic for Coil ImageLoader with Fresco APIs - Extracted ImageLoader to a separate file ## Test code and steps to reproduce Tested with BottomTabs example. ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <[email protected]>
* Remarks: `imageSource` type doesn't support SVGs on Android. | ||
* For loading SVGs use `drawableResource` type. |
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.
Does this mean that passing an SVG image as imageSource
will not work anymore?
Currently, passing an SVG icon like this works on Android, which is very handy:
<Icon
src={{
default: require('./icon.svgx'),
}}
/>
If only drawables are supported on Android, how are they supposed to be added when using a managed Expo workflow with CNG? Will there be an official config plugin to manage drawables? Either way, it seems like adding unnecessary complexity for the common use case of using SVG images as tab bar icons on Android.
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! This PR does not change that, it only better documents current state of the lib.
The changes have already landed in #3216.
With latest stable 4.16 version the svgs are supported as we used coil
lib for loading the resources, however we run into some issues, which led us to back out from that usage, migrating to fresco
which is managed by react-native
installation directly (from our PoV it comes bundled with the app), which does not have svg support.
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.
We're in touch with expo-router team, so that they're aware of this current limitation & can take appropriate steps on their end.
Description
This PR refactors the way icon resources are passed into components by merging platform-specific props into a single unified icon configuration structure. Previously, icons were defined using 3–4 separate props depending on the platform, which led to inconsistency and potential conflicts. Now, the icon is defined using a single object:
Platform-specific types (
PlatformIconAndroid
,PlatformIconIOS
,PlatformIconShared
) are used to define supported icon formats and sources per platform.Note:
This PR introduces a new approach to handling SVGs on Android. In #3216, I've started migrating to the Fresco image loading library, which unfortunately does not support SVGs directly.
As a result, SVG support will now be provided exclusively through the
drawableResourceAndroid
type.Changes
icon
prop.Test code and steps to reproduce
Verified that the updated
TestBottomTabs
example is showing icons as expected.Checklist