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

Fix custom fonts failed to load in react-native-vector-icons #267

Closed
wants to merge 4 commits into from

Conversation

Kudo
Copy link

@Kudo Kudo commented Aug 14, 2023

Why

react-native-vector-icons uses createIconSet(glyphmaps: Record<string, number>, fontName: string, fontFileName: string) to load custom fonts. the third parameter is passed in string and developers should include the font into native (learn more from the doc).

since we alias react-native-vector-icons to @expo/vector-icons, if people using bare react-native with custom fonts. our font loading logic will break react-native-vector-icons custom font.

How

our way to load the font is something like createIconSet(glyphmaps, fontName, require('./assets/MaterialIcons.ttf'). we can check the first parameter with AssetRegistry, if it is not a valid asset, we then fallback to the react-native-vector-icons use case.

to make the @ts-expect-error work, i have bumped typescript version inside expo-module-scripts in this pr.

Test Plan

tested the two cases on react-native 0.72 (adding the MaterialIcons2.ttf to xcode)

@expo/vector-icons classic code

const Icon = createIconSet(
  require('@expo/vector-icons/build/vendor/react-native-vector-icons/glyphmaps/FontAwesome.json'),
  'FontAwesome',
  require('@expo/vector-icons/build/vendor/react-native-vector-icons/Fonts/FontAwesome.ttf'),
);
<Icon name="work" size={30} color="#900" />

react-native-vector-icons custom fonts

const Icon = createIconSet(
  require('./assets/MaterialIcons.json'),
  'Material Icons',
  'MaterialIcons2.ttf',
);
<Icon name="comments" size={30} color="#900" />

@vercel
Copy link

vercel bot commented Aug 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
vector-icons ✅ Ready (Inspect) Visit Preview Aug 14, 2023 4:28pm

@Kudo Kudo requested a review from brentvatne August 14, 2023 16:30
@matinzd
Copy link

matinzd commented Aug 15, 2023

@@ -9,6 +9,8 @@ import {
TextStyle,
ViewStyle,
} from "react-native";
// @ts-expect-error: AssetRegistry is not typed
import { getAssetByID } from "react-native/Libraries/Image/AssetRegistry";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it better to use Image.resolveAssetSource() instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are not image assets

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I have used it for non images as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using Image.resolveAssetSource() here may work since we only check whether the result value is null. however, it will do more unnecessary/incorrect computations for images (not for fonts): https://github.com/facebook/react-native/blob/be2bb51c70a6885cd6607f0ea5df35f053645817/packages/react-native/Libraries/Image/AssetSourceResolver.js#L74-L86

Comment on lines +127 to +130
// If `getAssetByID` returns `null`, the call path is probably from react-native-vector-icons
// that the `expoAssetId` parameter is a string font name. e.g. `MaterialIcons.ttf`.
// We just pass the font to react-native-vector-icons without checking if it's loaded.
fontIsLoaded: Font.isLoaded(fontName) || !getAssetByID(expoAssetId),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's talk about this at our sync tmrw!

@Aleksandar-FFWD
Copy link

Any updates on this or workarounds to make custom fonts work in bare workflow?

@enfipy
Copy link

enfipy commented Oct 31, 2023

I don't really like to ping PRs but is there any chance that this will land soon?

@Kudo
Copy link
Author

Kudo commented Oct 31, 2023

sorry for being late to this pr. together with Brent we discussed an alternative solution than this pr. basically that Font.isLoaded(fontName) should support fonts added from native setup.
i will create a pr before sdk 50 and the issue should be fixed in sdk 50. does that make sense to you? or let me know if the issue is a blocker to you.

Kudo added a commit to expo/expo that referenced this pull request Dec 7, 2023
# Why

follow up with expo/vector-icons#267 (comment)

# How

add the custom native font support for `Font.isLoaded`

# Test Plan

- add a **icomoon.ttf** getting from https://icomoon.io/app/ to bare-expo as a custom font and add a test suite test case
  - also attach the download zip here as a reference [icomoon.zip](https://github.com/expo/expo/files/13576921/icomoon.zip)
- test react-native-vector-icon with the custom icomoon font
onizam95 pushed a commit to onizam95/expo-av-drm that referenced this pull request Jan 15, 2024
# Why

follow up with expo/vector-icons#267 (comment)

# How

add the custom native font support for `Font.isLoaded`

# Test Plan

- add a **icomoon.ttf** getting from https://icomoon.io/app/ to bare-expo as a custom font and add a test suite test case
  - also attach the download zip here as a reference [icomoon.zip](https://github.com/expo/expo/files/13576921/icomoon.zip)
- test react-native-vector-icon with the custom icomoon font
@brentvatne brentvatne closed this May 3, 2024
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

5 participants