-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[iOS][font] Fix some vector icons using incorrect fonts #28747
Conversation
The Pull Request introduced fingerprint changes against the base commit: 060e01d Fingerprint diff[
{
"type": "dir",
"filePath": "../../packages/expo-font/ios",
"reasons": [
"expoAutolinkingIos"
],
"hash": "f7dcef06a123de8ecd437c49239163de9a43e71d"
}
] Generated by PR labeler 🤖 |
6183020
to
d0edb10
Compare
<Ionicons name="logo-google" size={25} /> | ||
<Ionicons name="alarm" size={25} /> | ||
</View> | ||
|
||
<View style={{ paddingVertical: 10, paddingHorizontal: 15, flex: 1 }}> | ||
<Text style={{ fontFamily: 'space-mono', fontSize: 16 }}> |
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 might want to also add a font that we name something totally unrelated like "A-Custom-Font-Name" to verify that we don't regress in that functionality
hey please i have the same problem when i update sdk 51 this is my package.json { |
in android |
Why
Fixes #28693
How
When the swizzled
UIFont.fontNames(forFamilyName:)
function cannot find any font names for the aliased font family, we can assume it was not a family name but a font name in which case we can return an array with this font name instead of an empty array. Thanks to this, the font resolution in RN will not enter this if: https://github.com/facebook/react-native/blob/00b251b8ee8752b82a536faabc424d92f7034ebe/packages/react-native/React/Views/RCTFont.mm#L448 but will enter this one: https://github.com/facebook/react-native/blob/00b251b8ee8752b82a536faabc424d92f7034ebe/packages/react-native/React/Views/RCTFont.mm#L471 which is the expected behavior as it would bypass the unwanted logic.In other words, this change shifts the responsibility of handling font names passed as family names to us.
I also reverted #28407 which is no longer necessary and added some FontAwesome icons to our expo-font examples in native-component-list app.
Test Plan
Tested repros from #28693, #28407 and the new examples on both old and new architectures
Also thanks for @alanjhughes for helping me investigate this issue and test various cases and fonts.