Skip to content

Add fontStyle / fontName argument for FontAssetDelegate #2103

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

Merged
merged 4 commits into from
Jun 30, 2022

Conversation

jhen0409
Copy link
Contributor

In my case, using FontAssetDelegate to listen fetchFont(fontFamily) was not enough to know what font to load.

For example, I got fontFamily == "Roboto" but don't know the font style is Black. This PR will solve this problem.

@jhen0409 jhen0409 changed the title Add fontStyle argument for FontAssetDelegate Add fontStyle / fontName argument for FontAssetDelegate Jun 28, 2022
@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

This would be a breaking change. To preserve backwards compatibility, the new methods should be added and then in FontAssetManager, we'll need to call the old one followed by the new one if the former returns null.

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

@gpeal
Copy link
Collaborator

gpeal commented Jun 29, 2022

Could you test https://github.com/airbnb/lottie-android/blob/master/snapshot-tests/src/main/assets/Tests/EmbeddedFont.zip? It looks like, from the snapshot tests, that the embedded font is no longer loading in this PR.
CleanShot 2022-06-29 at 08 44 44

@jhen0409
Copy link
Contributor Author

I did some testing on local. I have no permission to use that Happo / AWS S3 feature, so I replaced some s3 upload logic on debug.

  • Currently I can't successfully render text, and the snapshot file size always 0 on local. Maybe have some guide for how to run snapshot-tests correctly?
  • It seems does get it's own font (by Font.getTypeface()), no different with master.

@jhen0409
Copy link
Contributor Author

Tested on sample (add EmbeddedFont.zip to assets). It seems works as expected (Pixel 6):

Screenshot (Jun 30, 2022 12_20_27 PM)

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

3 participants