react-native-asset icon indicating copy to clipboard operation
react-native-asset copied to clipboard

Support for Android font XML files

Open fwielstra opened this issue 1 year ago • 14 comments

Hi! This is probably half an issue, half a question, but here goes. TL;DR at the bottom.

We ran into an issue where we had a custom font called Filson, to be used in our branding; specifically, we had only one font file and weight, Filson Soft Black.

We had react-native-asset install it into our iOS and Android bundle. On iOS, it worked fine; I think iOS is very forgiving when it comes to fonts and specifically font weights, it used the correct font and weight.

But it seems Android is a bit more picky, and will fall back on the system font if the right font family, file, and font weights are not found.

After a lot of trial, error and head banging against the wall, I found a solution for the issue we had; following this guide, we added an XML file to the Android resource bundle alongside the font file that declared the font name (resource) and supported weights.

But, that xml file had to be put right into the Android folder of the specific app; when we put it alongside the font file in our shared resources folder (we have multiple apps in one repository btw), the font file is copied as-is but the accompanying XML file is copied to a "custom" folder.

TL;DR: Is it possible to copy Android font family declaration XML files to the same folder as the font files themselves? The current behaviour is that the accompanying XML files are copied to a 'custom' folder instead, which is not correct. (it might work, but I haven't actually tried it; the guide linked above indicates the XML files should be in the fonts folder)

fwielstra avatar Mar 29 '23 07:03 fwielstra

First can you try to see if adding the XML to a different folder (like custom) works?

unimonkiez avatar Apr 12 '23 21:04 unimonkiez

Hi 👋🏼

@fabioh8010 has been working on implementing this for a few weeks. It would have been better to open an issue like this one and engage @unimonkiez for feedback earlier, but hopefully we should be able to contribute XML font support to this repo soon. We're certainly happy to talk about it more.

roryabraham avatar Apr 17 '23 17:04 roryabraham

Hi @unimonkiez

Yes, as @roryabraham stated I have been working on an improvement for this library. We are aware that RN have some limitations regarding fonts in Android, e.g. fontStyle and fontWeight don't work reliably compared to iOS. Looking at this step-by-step guide, it's possible to configure the RN Android app in a way it will be able to use fonts the same way we can do in iOS.

So, our plan is to do the steps described in that guide in automatic way when using your library to link the Android assets. I'm finishing the implementation and will be able to raise a PR to your repo soon. With this new way of linking Android font assets, the community will benefit from a better experience while working with custom fonts.

You can read the detailed solution here.

fabioh8010 avatar Apr 19 '23 10:04 fabioh8010

Update: Working on the logic to update MainApplication.java file.

fabioh8010 avatar Apr 19 '23 21:04 fabioh8010

Update: Feature almost ready, doing some final tests now.

fabioh8010 avatar Apr 24 '23 21:04 fabioh8010

Update: Add more logic to handle new/updated fonts when you already have the XML files created by the library in the project. Implementing logic to handle deletions now.

fabioh8010 avatar Apr 25 '23 15:04 fabioh8010

Update: @unimonkiez @roryabraham I created a WIP Draft PR with the whole logic to copy and clean Android font assets using the new approach, but I have some doubts about one missing logic:

How should we handle the cases where the application has already linked the Android fonts in the old way?

My suggestion would be to create a CLI parameter to link/unlink the Android fonts in the new way, explaining in the docs that the user must unlink all fonts first in the old way before linking in the new way again, alongside with replacing the application logic to use the fonts in the same way iOS do now.

WDYT?

fabioh8010 avatar Apr 28 '23 07:04 fabioh8010

@fabioh8010 given that there's application code that needs to be changed as part of the migration, that sounds like a good approach to me 👍

roryabraham avatar Apr 28 '23 17:04 roryabraham

Just a note: I will be OOO for some days and will return on May 4th to start working on this final change once we got a consensus.

fabioh8010 avatar Apr 28 '23 18:04 fabioh8010

Hi @unimonkiez , did you have a chance to take a look at my comment above? wdyt about my suggestion?

fabioh8010 avatar May 04 '23 09:05 fabioh8010

I think if it's not too much hassle, add the cli version to the manifests and create a migration folder between each version, that way if the version is missing from the manifest it will know to run your migration, and any future version will be able to use that migration system. As for the application code that needs to be changed, some docs needs to be added on what needs to be changed in the code itself, can release this PR under a major update just to play it safe 🙌 What do you think?

unimonkiez avatar May 07 '23 21:05 unimonkiez

Hi @unimonkiez !

I think if it's not too much hassle, add the cli version to the manifests and create a migration folder between each version, that way if the version is missing from the manifest it will know to run your migration, and any future version will be able to use that migration system.

Not sure if I understood this, could you elaborate more about the migration system you proposed?

As for the application code that needs to be changed, some docs needs to be added on what needs to be changed in the code itself, can release this PR under a major update just to play it safe 🙌

Yes I think it's better to treat this as a major release, since it will be a very different and incompatible way of handling Android assets now.


About the PR, please feel free to review it when you have some time, in this way I can already address the review changes (if any) and the last past we have been discussing about.

fabioh8010 avatar May 11 '23 15:05 fabioh8010

Silly me, there is already a migration system, just need to use that

Edit: Left a review on #52

unimonkiez avatar May 17 '23 14:05 unimonkiez

Update: Addressed the changes and final implementation requested by maintainer.

fabioh8010 avatar May 20 '23 21:05 fabioh8010