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

[Android] Fix font weight numeric values #29117

Closed
wants to merge 16 commits into from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Jun 11, 2020

Summary

This issue fixes #25696 fixes #28854 fixes #26193
Since Android API 28 it is possible to specify fontWeight with numerical values ranging from 100 to 900

This pr uses the new Typeface.create() method available on Android API 28+ to set font weight value ranging from 100 to 900, while still keeping existing functionalities (custom fonts, bold/italic and other styles).
https://developer.android.com/reference/android/graphics/Typeface#create(android.graphics.Typeface,%20int,%20boolean)

Changelog

[Android] [Fixed] - Fix font weight numeric values

Test Plan

Works in all scenarios.

CLICK TO OPEN TESTS RESULTS

BEFORE AFTER
AFTER AFTER
AFTER

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 11, 2020
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Jun 11, 2020
@analysis-bot
Copy link

analysis-bot commented Jun 11, 2020

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,009,628 567
android hermes armeabi-v7a 6,673,226 573
android hermes x86 7,429,932 550
android hermes x86_64 7,320,815 541
android jsc arm64-v8a 9,169,354 446
android jsc armeabi-v7a 8,825,092 459
android jsc x86 9,017,756 466
android jsc x86_64 9,594,851 454

Base commit: 12543d5

@analysis-bot
Copy link

analysis-bot commented Jun 11, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 12543d5

@fangasvsass
Copy link

Looks good!

@fabOnReact fabOnReact marked this pull request as ready for review June 12, 2020 15:42
@JoshuaGross
Copy link
Contributor

@fabriziobertoglio1987 What about iOS?

@fabOnReact
Copy link
Contributor Author

@JoshuaGross The below are tests on iOS emulator from latest master branch (BEFORE) and from fix-font-weight branch (AFTER). I don't detect any issue on iOS.

Additionally I added a new commit e03864e to include same example on ios RNTester.
Please contact me for any further work, improvements or issues. I will immediately fix them.

Thanks a lot
I wish you a good day
Fabrizio Bertoglio

BEFORE AFTER
AFTER e03864e
AFTER AFTER

@LeeKyungJoon
Copy link

@fabriziobertoglio1987
Hey, it says your Merge has failed. As a result, fontWeight in android is still not applied to numeral values. Can you check it again?

@fabOnReact
Copy link
Contributor Author

Thanks for reviewing this pull request @LeeKyungJoon, currently I am working on another Pull Request full-time and I'm sending you a quick reply. I will come back to this pull request and re-execute all the tests/verify everything is working correctly in 2-3 days.

Hey, it says your Merge has failed. As a result, fontWeight in android is still not applied to numeral values. Can you check it again?

Your message is not not clear to me. I don't know if you are trying to implement this fix/pull request, in such case you will need to build ReactAndroid from source.

This pull request is not yet merged to master. Once the pull request is merged in the react-native master branch, you can expect it to be available as npm package as a release candidate for testing, then will be published as release. You can upgrade at that time react-native dependency and use the upgrade tool and use the numeric falues for the font. You can expect it to be available as npm package in 2-6 months.

Instead, If you are referring to potential merge conflicts from this commit e03864e and to the fact this branch was written on an Ubuntu Machine and then tested on MacBook machine. I did not make any changes to this pull request using the MacBook, but just checkout and test this branch. The commits were all written on my Ubuntu machine, as sometimes using 2 different machines to change the same branch will create merge conflicts. I rebased before publishing the Pull Request and all CI tests are passing.

Thanks a lot
I remain available for further help
Fabrizio Bertoglio

@LeeKyungJoon
Copy link

LeeKyungJoon commented Jul 23, 2020

@fabriziobertoglio1987
Thank you for your answer. I don't need it right now. However, I tried to use numeral values to implement more accurate fontWeight. I'm using the reaction-native 63.0 now, and I was wondering if it should be applied to this version. And if Merge fails in my question, I misunderstood the expression Merging is Blocked. I'm sorry.
"You can expect it to be available as npm package in 2-6 months."
I'll believe you and wait.
Thank you again for your answer.

@stackia
Copy link

stackia commented Jul 28, 2020

Typeface.create(typeface, weight, italic);

This is only available to Android API >= 28.

@fabOnReact
Copy link
Contributor Author

Typeface.create(typeface, weight, italic);

This is only available to Android API >= 28.

@stackia thanks a lot. You are right. I will try to fix it as soon as possible. Sorry. I wish you a good day. Fabrizio

@fabOnReact
Copy link
Contributor Author

fabOnReact commented Aug 1, 2020

@stackia Thanks a lot for your help

This is the screenshot of the functionality disabled on API < 28, changes included in b016126

@AlphaJuliettOmega
Copy link

It's bizarre to have only 'normal' and 'bold' font weights on Android but variable font sizes work on iOs
RN .64

on Android 700 defaults to 'Bold'
800+ becomes 'normal'

Now I have to implement platform specific font weights &/ somehow generate a font for each weight from a single font or disregard app designs to get the app to look similar on iOs + Android

@Kylar13
Copy link

Kylar13 commented May 25, 2021

Is there an update on this? How could we help get this through the door?

@facebook-github-bot
Copy link
Contributor

@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yungsters
Copy link
Contributor

Thank you for your contribution, @fabriziobertoglio1987. I just verified this locally (and made some notes for what this change means for Fabric) and expect it to be merged after Facebook CI succeeds.

@facebook-github-bot
Copy link
Contributor

@yungsters merged this pull request in 3827ca6.

@andreialecu
Copy link

If you'd like to help give this a better chance to be part of 0.64.3, upvote this comment for visibility in the upcoming v0.64.3 cherrypicks discussion:

react-native-community/releases#232 (comment)

kelset pushed a commit that referenced this pull request Jun 16, 2021
Summary:
This issue fixes #25696 fixes #28854 fixes #26193
Since Android API 28 it is possible to specify fontWeight with numerical values ranging from 100 to 900

This pr uses the new Typeface.create() method available on Android API 28+ to set font weight value ranging from 100 to 900, while still keeping existing functionalities (custom fonts, bold/italic and other styles).
https://developer.android.com/reference/android/graphics/Typeface#create(android.graphics.Typeface,%20int,%20boolean)

## Changelog

[Android] [Fixed] - Fix font weight numeric values

Pull Request resolved: #29117

Test Plan:
Works in all scenarios.

**<details><summary>CLICK TO OPEN TESTS RESULTS</summary>**
<p>

| **BEFORE** | **AFTER** |
|:-------------------------:|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84420949-1daa0e80-ac1b-11ea-9a2e-eaac03dc4533.png"  width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/84490766-edf31900-aca3-11ea-90d8-7c52d2e2be59.png" width="300" height="" /> |

| **AFTER** | **AFTER** |
|:-------------------------:|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84490768-ee8baf80-aca3-11ea-8d3e-937d87b3c56a.png"  width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/84490769-ef244600-aca3-11ea-9dec-5eb70358834b.png" width="300" height="" /> |

| **AFTER** |
|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84490772-f0557300-aca3-11ea-851a-5befc900192c.png"  width="300" height="" />|

</p>
</details>

Reviewed By: lunaleaps

Differential Revision: D28917328

Pulled By: yungsters

fbshipit-source-id: 8b84e855b3a8b87960cb79b9237d452b26974c36
Setito pushed a commit to Setito/react-native that referenced this pull request Jul 17, 2021
Summary:
This issue fixes facebook#25696 fixes facebook#28854 fixes facebook#26193
Since Android API 28 it is possible to specify fontWeight with numerical values ranging from 100 to 900

This pr uses the new Typeface.create() method available on Android API 28+ to set font weight value ranging from 100 to 900, while still keeping existing functionalities (custom fonts, bold/italic and other styles).
https://developer.android.com/reference/android/graphics/Typeface#create(android.graphics.Typeface,%20int,%20boolean)

## Changelog

[Android] [Fixed] - Fix font weight numeric values

Pull Request resolved: facebook#29117

Test Plan:
Works in all scenarios.

**<details><summary>CLICK TO OPEN TESTS RESULTS</summary>**
<p>

| **BEFORE** | **AFTER** |
|:-------------------------:|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84420949-1daa0e80-ac1b-11ea-9a2e-eaac03dc4533.png"  width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/84490766-edf31900-aca3-11ea-90d8-7c52d2e2be59.png" width="300" height="" /> |

| **AFTER** | **AFTER** |
|:-------------------------:|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84490768-ee8baf80-aca3-11ea-8d3e-937d87b3c56a.png"  width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/84490769-ef244600-aca3-11ea-9dec-5eb70358834b.png" width="300" height="" /> |

| **AFTER** |
|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84490772-f0557300-aca3-11ea-851a-5befc900192c.png"  width="300" height="" />|

</p>
</details>

Reviewed By: lunaleaps

Differential Revision: D28917328

Pulled By: yungsters

fbshipit-source-id: 8b84e855b3a8b87960cb79b9237d452b26974c36
@tushar-singhal
Copy link

this will be the part of 0.66 🎉

@manuhook
Copy link

this will be the part of 0.66 🎉

Seems it's already in 0.65.x

@enzzoperez
Copy link

enzzoperez commented Oct 16, 2021

Hi!, thanks for the PR, I recently migrated from 0.61 to 0.65.1 and only words (normal,bold, bolder, etc) in fontWeight for my custom fonts works.. 🤔 , is there any aditional config to add about the fonts?

@fabOnReact
Copy link
Contributor Author

@enzzoperez I'll prepare a fix after 15th November #26193 (comment) thanks

@darkbasic
Copy link

I'll prepare a fix after 15th November #26193 (comment) thanks

Does it mean it still doesn't work? I thought this was supposed to fix the issue.

@fabOnReact
Copy link
Contributor Author

@darkbasic sorry. I used the wrong word. It is not a fix. The functionality works, but I would like to improve it so that it's easier to use and more intuitive. Developers should not end up reading the internal docs, prs and issues to understand it. Unluckily, I don't have any more savings in my bank account and I need to work right now, I had to give priorities to other activities as OpenSouce does not allow me to pay my basic expenses. For this reason there is a delay of 1.5 months. Thanks a lot.

danilobuerger pushed a commit to feastr/react-native that referenced this pull request Oct 28, 2021
Summary:
This issue fixes facebook#25696 fixes facebook#28854 fixes facebook#26193
Since Android API 28 it is possible to specify fontWeight with numerical values ranging from 100 to 900

This pr uses the new Typeface.create() method available on Android API 28+ to set font weight value ranging from 100 to 900, while still keeping existing functionalities (custom fonts, bold/italic and other styles).
https://developer.android.com/reference/android/graphics/Typeface#create(android.graphics.Typeface,%20int,%20boolean)

## Changelog

[Android] [Fixed] - Fix font weight numeric values

Pull Request resolved: facebook#29117

Test Plan:
Works in all scenarios.

**<details><summary>CLICK TO OPEN TESTS RESULTS</summary>**
<p>

| **BEFORE** | **AFTER** |
|:-------------------------:|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84420949-1daa0e80-ac1b-11ea-9a2e-eaac03dc4533.png"  width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/84490766-edf31900-aca3-11ea-90d8-7c52d2e2be59.png" width="300" height="" /> |

| **AFTER** | **AFTER** |
|:-------------------------:|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84490768-ee8baf80-aca3-11ea-8d3e-937d87b3c56a.png"  width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/84490769-ef244600-aca3-11ea-9dec-5eb70358834b.png" width="300" height="" /> |

| **AFTER** |
|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84490772-f0557300-aca3-11ea-851a-5befc900192c.png"  width="300" height="" />|

</p>
</details>

Reviewed By: lunaleaps

Differential Revision: D28917328

Pulled By: yungsters

fbshipit-source-id: 8b84e855b3a8b87960cb79b9237d452b26974c36
danilobuerger pushed a commit to feastr/react-native that referenced this pull request Oct 28, 2021
Summary:
This issue fixes facebook#25696 fixes facebook#28854 fixes facebook#26193
Since Android API 28 it is possible to specify fontWeight with numerical values ranging from 100 to 900

This pr uses the new Typeface.create() method available on Android API 28+ to set font weight value ranging from 100 to 900, while still keeping existing functionalities (custom fonts, bold/italic and other styles).
https://developer.android.com/reference/android/graphics/Typeface#create(android.graphics.Typeface,%20int,%20boolean)

## Changelog

[Android] [Fixed] - Fix font weight numeric values

Pull Request resolved: facebook#29117

Test Plan:
Works in all scenarios.

**<details><summary>CLICK TO OPEN TESTS RESULTS</summary>**
<p>

| **BEFORE** | **AFTER** |
|:-------------------------:|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84420949-1daa0e80-ac1b-11ea-9a2e-eaac03dc4533.png"  width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/84490766-edf31900-aca3-11ea-90d8-7c52d2e2be59.png" width="300" height="" /> |

| **AFTER** | **AFTER** |
|:-------------------------:|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84490768-ee8baf80-aca3-11ea-8d3e-937d87b3c56a.png"  width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/84490769-ef244600-aca3-11ea-9dec-5eb70358834b.png" width="300" height="" /> |

| **AFTER** |
|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/84490772-f0557300-aca3-11ea-851a-5befc900192c.png"  width="300" height="" />|

</p>
</details>

Reviewed By: lunaleaps

Differential Revision: D28917328

Pulled By: yungsters

fbshipit-source-id: 8b84e855b3a8b87960cb79b9237d452b26974c36
@MauriceArikoglu
Copy link

@fabriziobertoglio1987 thanks for caring 👍

@fabOnReact
Copy link
Contributor Author

@MauriceArikoglu I'll try to find free time to work on improving this functionality in the future, but will take some months. Thanks a lot

@pke
Copy link

pke commented Nov 17, 2022

This still doesn't work. I wonder if no one at facebook is using bold font faces in their apps?

@fabOnReact
Copy link
Contributor Author

fabOnReact commented Nov 17, 2022

#26193 (comment)
did not have any free time to work on this. sorry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Needs: React Native Team Attention Platform: Android Android applications.
Projects
None yet