Fix android letter spacing #9420
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
|
@tim-ford updated the pull request. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
ghost
added
the
CLA Signed
|
awesome |
|
@tim-ford updated the pull request. |
|
@tim-ford updated the pull request - view changes |
ghost
added
the
CLA Signed
|
Any update on this? |
| int fontSize = update.getFontSize(); | ||
| if (!FloatUtil.floatsEqual(mLetterSpacing, nextLetterSpacing)) { | ||
| mLetterSpacing = nextLetterSpacing; | ||
| if(Float.isNaN(mLetterSpacing)) { |
|
Thanks for adding this! In the Test plan, could you please show screenshots of text that has no letterSpacing set (should be unchanged), and a few examples of text with a few different values of letterSpacing set? This PR sets the letter spacing in |
ghost
added
the
CLA Signed
|
@tim-ford do you have any updates for this pull request? It's been a while since the last update so wanted to check in and see if you've looked at the requested changes. |
ghost
added
the
CLA Signed
|
Will this be compatible with API 19+? |
|
the formula and fixes work -- the formula was researched on the web for PX to EM conversions since Android does letterSpacing in EM -- the reason this hasn't gone anywhere is: not sure why it fails the tests -- since everything works fine in a functional App |
|
Also have updated fork to 35-stable -- not sure how to bring in those adjustments so the files can no longer in conflice |
|
Any news on this, guys? |
|
@tim-ford Are the tests failing for you locally too, or do you think it's something going wrong with CircleCI? I am not sure what you mean exactly when you say "not sure why it fails the tests" |
|
its been a couple of weeks since looking -- I have already released an application with this fix in it, the fix is fine -- updates have occurred since that will require some adjustments to code placement -- It seemed like an issue with CI at the time |
|
The test errors do look related to spacing on Android so it at least isn't obvious to me that it's a flaky CI problem. But take a look and see if you agree with my take. Also fixing the merge conflicts should "kick" the CI to run again anyway. |
|
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Importing to see if our internal tests pass. It does seem like there are some conflicts that need to be resolved before we can merge this though. |
|
@tim-ford can you resolve the conflicts? |
|
Would love this feature request to be resolved :) |
|
It seems like this pull request has been abandoned. I'm going to close it but if you are still interested in working on this please feel free to reopen! |
|
Why was this pull request abandoned? |
|
Checkout #13199. I implemented letterSpacing based on this PR, but also solve that the layout was calculated correctly, which was missed here. |
Motivation for this fix is to get "letterSpacing" property in styles to work on Android (for the most part - there are non working edge cases potentially - primary use case seems fine)
Test plan (required)
Tested using:
letterSpacing: -6,
fontSize:24,
fontFamily:'ionicons',
Good Stars
Bad Stars
