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

Support for Android font XML files #48

Open
fwielstra opened this issue Mar 29, 2023 · 14 comments
Open

Support for Android font XML files #48

fwielstra opened this issue Mar 29, 2023 · 14 comments

Comments

@fwielstra
Copy link

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)

@unimonkiez
Copy link
Owner

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

@roryabraham
Copy link
Contributor

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.

@fabioh8010
Copy link

fabioh8010 commented Apr 19, 2023

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
Copy link

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

@fabioh8010
Copy link

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

@fabioh8010
Copy link

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
Copy link

fabioh8010 commented Apr 28, 2023

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?

@roryabraham
Copy link
Contributor

roryabraham commented Apr 28, 2023

@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 👍

@fabioh8010
Copy link

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
Copy link

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

@unimonkiez
Copy link
Owner

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?

@fabioh8010
Copy link

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.

@unimonkiez
Copy link
Owner

unimonkiez commented May 17, 2023

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

Edit:
Left a review on #52

@fabioh8010
Copy link

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

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

No branches or pull requests

4 participants