Fix android letter spacing by tim-ford · Pull Request #9420 · react/react-native

tim-ford · 2016-08-16T01:20:59Z

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

ghost · 2016-08-16T01:21:24Z

By analyzing the blame information on this pull request, we identified @MengjueW and @rigdern to be potential reviewers.

ghost · 2016-08-16T01:21:26Z

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!

ghost · 2016-08-16T01:25:02Z

@tim-ford updated the pull request.

ghost · 2016-08-16T01:25:18Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

nihgwu · 2016-08-16T03:58:32Z

awesome

ghost · 2016-08-16T10:48:32Z

@tim-ford updated the pull request.

ghost · 2016-08-16T10:49:58Z

@tim-ford updated the pull request - view changes

munzur · 2016-09-03T22:38:23Z

Any update on this?

mkonicek · 2016-09-08T23:29:42Z

+ int fontSize = update.getFontSize();
+ if (!FloatUtil.floatsEqual(mLetterSpacing, nextLetterSpacing)) {
+ mLetterSpacing = nextLetterSpacing;
+ if(Float.isNaN(mLetterSpacing)) {


nit: space after if

mkonicek · 2016-09-08T23:32:39Z

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 setText - what's the reason of doing it there rather than in setLetterSpacing?

ghost · 2016-09-25T17:01:05Z

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

belohlavek · 2016-09-27T01:44:07Z

Will this be compatible with API 19+?
How long until it gets implemented?

tim-ford · 2016-09-28T07:44:41Z

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

tim-ford · 2016-09-28T07:45:30Z

Also have updated fork to 35-stable -- not sure how to bring in those adjustments so the files can no longer in conflice

hubciorz · 2016-10-19T10:34:30Z

Any news on this, guys?

lacker · 2016-10-25T00:37:18Z

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

tim-ford · 2016-10-25T01:21:23Z

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

lacker · 2016-10-25T04:51:45Z

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.

facebook-github-bot · 2016-11-07T21:58:53Z

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

hramos · 2016-11-07T21:59:18Z

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.

hramos · 2016-11-10T02:09:43Z

@tim-ford can you resolve the conflicts?

julesmoretti · 2016-11-18T23:25:05Z

Would love this feature request to be resolved :)

lacker · 2016-12-14T23:57:10Z

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!

jkomyno · 2017-03-24T22:58:27Z

Why was this pull request abandoned?

christoph-jerolimov · 2017-03-29T14:17:21Z

Checkout #13199. I implemented letterSpacing based on this PR, but also solve that the layout was calculated correctly, which was missed here.

tim-ford added 2 commits August 15, 2016 21:49

Android Letter Spacing fix
d80c698

LETTER SPACING CONSTANT
bd5c402

ghost added the GH Review: review-needed label Aug 16, 2016

Merge branch 'master' into fix-android-letter-spacing
f3d931a

ghost 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 Aug 16, 2016

Merge branch 'master' into fix-android-letter-spacing
5dab842

ghost 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 Aug 16, 2016

mkonicek added the GH Review: needs-revision label Sep 8, 2016

mkonicek reviewed Sep 8, 2016
View reviewed changes

ghost 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 Sep 8, 2016

maraujop mentioned this pull request Sep 22, 2016
[Android] TextView doesn't support letter spacing and padding #3233
Closed

ghost 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 Sep 25, 2016

lacker removed the GH Review: review-needed label Oct 25, 2016

leeight mentioned this pull request Oct 29, 2016
[android]the Textattri letterSpacing is neverworked #10622
Closed

lacker closed this Dec 14, 2016

zezhouliu mentioned this pull request Jan 24, 2017
Add lineSpacing prop to Android. airbnb/react-native#8
Closed

zezhouliu mentioned this pull request Mar 29, 2017
Enable linespacing for versions >= Lollipop (5.0) airbnb/react-native#13
Merged

christoph-jerolimov mentioned this pull request Mar 29, 2017
[Android] Implement letterSpacing option #13199
Closed

zezhouliu mentioned this pull request May 9, 2017
Add letter spacing for Android versions >= Lollipop (5.0). #13877
Closed
4 tasks

Uh oh!

Conversation

tim-ford commented Aug 16, 2016

Uh oh!

ghost commented Aug 16, 2016

Uh oh!

ghost commented Aug 16, 2016

Uh oh!

ghost commented Aug 16, 2016

Uh oh!

ghost commented Aug 16, 2016

Uh oh!

nihgwu commented Aug 16, 2016

Uh oh!

ghost commented Aug 16, 2016

Uh oh!

ghost commented Aug 16, 2016

Uh oh!

munzur commented Sep 3, 2016

Uh oh!

mkonicekSep 8, 2016

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Sep 8, 2016

Uh oh!

ghost commented Sep 25, 2016

Uh oh!

belohlavek commented Sep 27, 2016

Uh oh!

tim-ford commented Sep 28, 2016

Uh oh!

tim-ford commented Sep 28, 2016

Uh oh!

hubciorz commented Oct 19, 2016

Uh oh!

lacker commented Oct 25, 2016

Uh oh!

tim-ford commented Oct 25, 2016

Uh oh!

lacker commented Oct 25, 2016

Uh oh!

facebook-github-bot commented Nov 7, 2016

Uh oh!

hramos commented Nov 7, 2016

Uh oh!

hramos commented Nov 10, 2016

Uh oh!

julesmoretti commented Nov 18, 2016

Uh oh!

lacker commented Dec 14, 2016

Uh oh!

jkomyno commented Mar 24, 2017

Uh oh!

christoph-jerolimov commented Mar 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants