-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Feat/precompile fonts #3003
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: master
Are you sure you want to change the base?
Feat/precompile fonts #3003
Changes from 4 commits
07c018e
f7aca3f
104c144
f25a59a
0320212
5bbf8dd
2acb4c1
17ffdf2
a5ae257
5c97a05
2c98262
d75e7a1
13561b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ if (__DEV__) { | |
| import "./utils/gestureHandler" | ||
|
|
||
| import { useEffect, useState } from "react" | ||
| import { Platform } from "react-native" | ||
| import { useFonts } from "expo-font" | ||
| import * as Linking from "expo-linking" | ||
| import { KeyboardProvider } from "react-native-keyboard-controller" | ||
|
|
@@ -29,7 +30,7 @@ import { initI18n } from "./i18n" | |
| import { AppNavigator } from "./navigators/AppNavigator" | ||
| import { useNavigationPersistence } from "./navigators/navigationUtilities" | ||
| import { ThemeProvider } from "./theme/context" | ||
| import { customFontsToLoad } from "./theme/typography" | ||
| import { customFontsToLoadWebOnly } from "./theme/typography" | ||
| import { loadDateFnsLocale } from "./utils/formatDate" | ||
| import * as storage from "./utils/storage" | ||
|
|
||
|
|
@@ -68,7 +69,11 @@ export function App() { | |
| isRestored: isNavigationStateRestored, | ||
| } = useNavigationPersistence(storage, NAVIGATION_PERSISTENCE_KEY) | ||
|
|
||
| const [areFontsLoaded, fontLoadError] = useFonts(customFontsToLoad) | ||
| // We load fonts dynamically for web only, the rest are handled by | ||
| // the expo-font config plugin in `app.json`. If not using web, | ||
| // you can delete this permissive check along with associated | ||
| // code in `typography'. | ||
| const [areFontsLoadedWebOnly, fontLoadErrorWebOnly] = useFonts(customFontsToLoadWebOnly) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: we had been talking about this internally for a bit before your PR and were looking at an approach similar to how Evan Bacon and Expo are handling fonts cross platform. Rather than deleting for web, I think it would be ideal to gracefully handle web without having users make a choice. One example @frankcalise found was this Used like this: I'd prefer that implementation, myself.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! I wouldn't have come up with that approach, no matter how much time I was given, I haven't studied the React 19 APIs very much. This would probably include moving the i18n load permissive to the suspense fallback as well right? You want me to pursue that approach and see where it leads? |
||
| const [isI18nInitialized, setIsI18nInitialized] = useState(false) | ||
|
|
||
| useEffect(() => { | ||
|
|
@@ -83,7 +88,11 @@ export function App() { | |
| // In iOS: application:didFinishLaunchingWithOptions: | ||
| // In Android: https://stackoverflow.com/a/45838109/204044 | ||
| // You can replace with your own loading component if you wish. | ||
| if (!isNavigationStateRestored || !isI18nInitialized || (!areFontsLoaded && !fontLoadError)) { | ||
| if ( | ||
| !isNavigationStateRestored || | ||
| !isI18nInitialized || | ||
| (!areFontsLoadedWebOnly && !fontLoadErrorWebOnly && Platform.OS === "web") | ||
| ) { | ||
| return null | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| // markdown file and add links from here | ||
|
|
||
| import { Platform } from "react-native" | ||
| import type { FontSource } from "expo-font" | ||
| import { | ||
| SpaceGrotesk_300Light as spaceGroteskLight, | ||
| SpaceGrotesk_400Regular as spaceGroteskRegular, | ||
|
|
@@ -10,22 +11,48 @@ import { | |
| SpaceGrotesk_700Bold as spaceGroteskBold, | ||
| } from "@expo-google-fonts/space-grotesk" | ||
|
|
||
| export const customFontsToLoad = { | ||
| spaceGroteskLight, | ||
| spaceGroteskRegular, | ||
| spaceGroteskMedium, | ||
| spaceGroteskSemiBold, | ||
| spaceGroteskBold, | ||
| } | ||
| export const customFontsToLoadWebOnly = | ||
| Platform.OS === "web" | ||
| ? { | ||
| spaceGroteskLight, | ||
| spaceGroteskRegular, | ||
| spaceGroteskMedium, | ||
| spaceGroteskSemiBold, | ||
| spaceGroteskBold, | ||
| } | ||
| : ({} as Record<string, FontSource>) | ||
|
|
||
| const fonts = { | ||
| spaceGrotesk: { | ||
| // Cross-platform Google font. | ||
| light: "spaceGroteskLight", | ||
| normal: "spaceGroteskRegular", | ||
| medium: "spaceGroteskMedium", | ||
| semiBold: "spaceGroteskSemiBold", | ||
| bold: "spaceGroteskBold", | ||
| // The way expo-fonts config plugin applies | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: thanks for figuring this out! I haven't tried out the new font loading technique so it's good to learn this. |
||
| // fonts to the individual platforms, the names come out different | ||
| // on ios and android. For web, we have to load fonts asynchronously | ||
| // using useFonts. | ||
| light: Platform.select({ | ||
| ios: "SpaceGrotesk-Light", | ||
| android: "SpaceGrotesk-300Light", | ||
| web: "spaceGroteskLight", | ||
| }), | ||
| normal: Platform.select({ | ||
| ios: "SpaceGrotesk-Regular", | ||
| android: "SpaceGrotesk-400Regular", | ||
| web: "spaceGroteskRegular", | ||
| }), | ||
| medium: Platform.select({ | ||
| ios: "SpaceGrotesk-Medium", | ||
| android: "SpaceGrotesk-500Medium", | ||
| web: "spaceGroteskMedium", | ||
| }), | ||
| semiBold: Platform.select({ | ||
| ios: "SpaceGrotesk-SemiBold", | ||
| android: "SpaceGrotesk-600SemiBold", | ||
| web: "spaceGroteskSemiBold", | ||
| }), | ||
| bold: Platform.select({ | ||
| ios: "SpaceGrotesk-Bold", | ||
| android: "SpaceGrotesk-700Bold", | ||
| web: "spaceGroteskBold", | ||
| }), | ||
| }, | ||
| helveticaNeue: { | ||
| // iOS only font. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||
| { | ||||||
| "name": "ignite-cli", | ||||||
| "version": "11.2.0", | ||||||
| "version": "11.2.1", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
suggestion: no need for this, I think CI will do it for us.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: let's undo this change before merge, please. |
||||||
| "description": "Infinite Red's hottest boilerplate for React Native.", | ||||||
| "bin": { | ||||||
| "ignite": "bin/ignite", | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.