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

Improve Android fonts linking process #52

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

fabioh8010
Copy link

This PR aims to improve the whole Android fonts linking process.

Problem

Currently, during Android linking, font assets are copied to Android asset's folder and then can be used in the application. The problem is that in RN Android has a different logic to manage fonts related to iOS, requiring the developer to rely on the font file's name to be able to use the font properly in the application.

To give an example, let's say I added Lato font to my project and linked the files in both Android and iOS platforms. These are the Lato font files:

  • Lato-Black.ttf
  • Lato-BlackItalic.ttf
  • Lato-Bold.ttf
  • Lato-BoldItalic.ttf
  • Lato-Italic.ttf
  • Lato-Light.ttf
  • Lato-LightItalic.ttf
  • Lato-Regular.ttf
  • Lato-Thin.ttf
  • Lato-ThinItalic.ttf

For iOS, I can simply use a combination of fontFamily, fontWeight and fontStyle styles to select the desired font:

<Text style={{ fontFamily: 'Lato', fontWeight: '700', fontStyle: 'italic' }}>Lato 700 Italic</Text>

For Android, this approach won't work and you have to rely on the font's name in order to select the desired font.

<Text style={{ fontFamily: 'Lato-BoldItalic' }}>Lato 700 Italic</Text>

Developers end up having to use different approaches in order to custom fonts work properly for both platforms.

Solution

This PR solves the stated problem by changing the Android linking process to do the steps described in this guide automatically. In Android, fonts are now going to be managed by using XML Fonts to handle all font's variants.

During font assets copying process, here is the following logic:

  1. Read all font assets and create a font family map, where each entry is a different font family with its corresponding font files and its styles / weights.
  2. For each entry in the font family map:
    1. Create a new XML Font file or edit an existing one, replacing already defined entries and style / weight duplicates.
    2. Copy the font files to res/font/ folder and normalize their names.
    3. Add ReactFontManager's import to MainApplication.java file if it isn't declared yet.
    4. Add a method call inside onCreate() to load the custom font on Android.

During font assets cleaning process, here is the following logic:

  1. For each deleted font file, locate the corresponding one in res/font folder that needs to be deleted.
  2. Create a font family map, where each entry is a different font family with its corresponding font files.
  3. For each entry in the font family map:
    1. Remove the font entries in the corresponding font family XML file.
    2. If the XML file is empty, remove the file and the method call in MainApplication.java.
    3. If there is no usages of ReactFontManager in MainApplication.java, remove the import as well.

@fabioh8010 fabioh8010 marked this pull request as ready for review May 11, 2023 15:01
@fabioh8010 fabioh8010 changed the title [WIP] Improve Android fonts linking process Improve Android fonts linking process May 11, 2023
Copy link
Owner

@unimonkiez unimonkiez left a comment

Choose a reason for hiding this comment

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

Seems overall good and almost ready for merge!

  • My comments are mainly nitpicks, but are easy to fix.
  • Resolve conflict with master by rebase, since PR's are usually squash merged to master, I suggest when the conflict comes up to discard your yarn.lock and package.json changes and just reinstall everything using yarn.
  • The main issue is that I want the transition to the new version (probably 3.0.0) to be seemless, and for that we need to unlink and link again the font assets, try to add a migration that does that.
    • currently each migration just takes old manifests and returns new ones if something has changed in it's structure, but in this PR you don't need to change the structure, just link and unlink the fonts, maybe add a way to do that in the newly created migration for this PR

lib/helper.js Outdated Show resolved Hide resolved
lib/get-config.js Outdated Show resolved Hide resolved
lib/get-config.js Outdated Show resolved Hide resolved
lib/font-assets-helper.js Outdated Show resolved Hide resolved
lib/font-assets-helper.js Outdated Show resolved Hide resolved
lib/font-assets-helper.js Outdated Show resolved Hide resolved
lib/copy-assets/android.js Outdated Show resolved Hide resolved
lib/copy-assets/android.js Outdated Show resolved Hide resolved
lib/copy-assets/android.js Outdated Show resolved Hide resolved
@fabioh8010
Copy link
Author

@unimonkiez Thanks for your review! About the migration, even if I implement it won't be 100% seamless as it would require changes in the application as well, specifically how the developer use fontFamily, fontStyle and fontWeight. One thing we could do is display a message in the terminal explaining about the migration made and the necessary steps to update application's code as well, maybe even with a link to a doc file or something. WDYT?

@unimonkiez
Copy link
Owner

@unimonkiez Thanks for your review! About the migration, even if I implement it won't be 100% seamless as it would require changes in the application as well, specifically how the developer use fontFamily, fontStyle and fontWeight. One thing we could do is display a message in the terminal explaining about the migration made and the necessary steps to update application's code as well, maybe even with a link to a doc file or something. WDYT?

Sounds excellent !

@fabioh8010 fabioh8010 force-pushed the feature/android-font-family-improvement branch from 2c1c42f to a902630 Compare May 20, 2023 21:29
@fabioh8010
Copy link
Author

fabioh8010 commented May 20, 2023

@unimonkiez I updated the PR with the following changes:

  • Addressed the requested changes
  • Implemented a migration for the new Android font linking logic. Tested with edge cases as well e.g. migrating while adding/removing fonts
  • Added documentation and fixed some typos
  • Improved font italic detection logic to cover an edge case

Changes: https://github.com/unimonkiez/react-native-asset/pull/52/files/5952050efc5ab27f4592a85ce5ba843ce56d65c0..a902630a1313c3a0c6116937e0a2f56cf9f0215c

@fabioh8010
Copy link
Author

Hi @unimonkiez , did you have a chance to take a look at the latest changes on this PR? Thanks!

@fabioh8010
Copy link
Author

Hi @unimonkiez, just bringing this PR to your attention again, I appreciate any feedback or action you could take. Thanks!

@roryabraham
Copy link
Contributor

@unimonkiez thanks for your reviews of this pull request so far. We'd love to help push this feature forward in whatever way we can 🙂

@Vi-dot
Copy link

Vi-dot commented Aug 11, 2023

Thanks for your work @fabioh8010 and @unimonkiez
It works really well.

If anyone wants to use it now:

  1. Uninstall any previous version
npm uninstall -g react-native-asset
npm uninstall -D react-native-asset
  1. Install version from fabioh8010
npm i -D github:fabioh8010/react-native-asset#feature/android-font-family-improvement

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.

4 participants