-
-
Notifications
You must be signed in to change notification settings - Fork 611
feat(iOS): add xcassets support #3443
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
…managerDelegate.java
|
Hey! Thanks for the PR. We'll look into it after 4.19.0 release |
|
Thank you @kkafar ! I'll add more tests |
|
Hi @kkafar @kligarski , any chance to review this PR? Thank you |
|
Hey @ajanuar, it's in our task queue for January. We'll look into it. |
|
Hi, @ajanuar. Thanks for the PR and sorry for the delay. I'll push some changes and modify the description a bit. Hopefully, we'll be ready to land this soon. |
| iconImageSource?: ImageSource; | ||
| iconSfSymbolName?: string; | ||
| iconResourceName?: string; | ||
|
|
||
| selectedIconImageSource?: ImageSource; | ||
| selectedIconSfSymbolName?: string; | ||
| selectedIconResourceName?: string; |
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'm not sure about the name of this prop (iconResourceName). It's used for both sfsymbols and xcassets. We also have drawableIconResourceName and imageIconResource on Android which have very similar name.
Maybe those props should be unified (iconImageSource (removing imageIconResource), iconResourceName (removing drawableIconResourceName) for both platforms and selectedIconImageSource, selectedIconResourceName as iOS-only).
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 don't like this solution (adding new prop to pass xcasset's name), we could use only one prop for both sfsymbolname and xcassetname however we're currently not passing iconType so we can't differentiate between the two.
At least we're not passing it intentionally - we're using ...icon so it actually is present on the native side but I don't want to rely on that. We need to refactor this logic and add proper typing in JS layer because it's very difficult to add any changes now without being afraid of this whole feature breaking apart.
Decided to go with separate prop for now as refactoring this logic would be way out of scope of this PR.
Description
This PR adds support for using .xcassets asset catalog icons across key navigation UI elements. Developers can now reference native asset names directly for both bottom-tab icons and header bar button items, removing the need to bundle standalone image files. By integrating with iOS/macOS asset catalogs, the update aligns icon handling with platform best practices, simplifies asset management, and enables automatic support for multi-resolution and variant images (such as dark mode).
Addresses discussion.
Closes https://github.com/software-mansion/react-native-screens-labs/issues/576.
Usage
You can add logo to asset catalog via Xcode:
Changes
xcasseticon type for iOSxcassetfor Tabsxcassetfor Bar Button Items in StackBarButtonItemsexample screen and addTest3443that tests xcassets in Bottom TabsTest3443should be converted in the future into single feature test when chore: Refactor library examples & tests #3528 is mergedBefore & after - visual documentation
Test code and steps to reproduce
Run
BarButtonItemsexample screen. UseXcassetIconButtonDemoandAdvancedMenuButtonDemo.Run
Test3443.Checklist