Document letterSpacing, including on Android #105
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We shouldn't merge this PR until facebook/react-native#17398 is merged. |
charpeni
added
the
🕐 Waiting on PR
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Feb 27, 2018
Summary: `letterSpacing` is completely missing from RN Android at the moment. I've reviewed the `letterSpacing` implementations in #13199, #13877 and #16801 (that all seem to have stalled) and managed to put together an improved one based on #13199, updated to merge cleanly post 6114f86, that resolves the [issues](#13199 (comment)) I've identified with that code. I believe this is the closest PR yet to a correct implementation of this feature, with a few caveats: - As with the other PRs, this only works on Android >= 5.0 (silently falling back to no letter spacing on older versions). Is this acceptable for a RN feature, in general? Would a dev mode warning be desirable? - The other PRs seem to have explored the space of potential solutions to the layout issue ([Android renders space _around_ glyphs](https://issuetracker.google.com/issues/37079859), iOS to the _right_ of each one) and come up empty, so I've opted to merely document the difference. - I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers. - The implementation in `ReactEditText` is only there to handle the placeholder text, as `ReactBaseTextShadowNode` already affects the input control's contents correctly. - I'm not sure whether `<TextInput>` is meant to respect `allowFontScaling`; I've taken my cue here from `ReactTextInputManager.setFontSize()`, and used the same units (SP) to interpret the value in `ReactEditText.setLetterSpacingPt()`. - I'm not sure whether `<TextInput>` is even meant to support `letterSpacing` - it doesn't actually work on iOS. I'm not going to be able to handle the Objective-C side of this, not as part of this PR at least. - I have not added unit tests to `ReactTextTest` - is this desirable? I see that some other props such as `lineHeight` aren't covered there (unless I'm not looking in the right place). - Overall, I'm new to this codebase, so it's likely I've missed something not mentioned here. Note comment re: unit tests above; RNTester screenshots follow. | iOS (existing functionality, amended test) | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458459-c8d59498-edcb-11e7-8c8f-e7426f723886.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458473-2a1ca368-edcc-11e7-9ce6-30c6d3a48660.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458481-6c60a36e-edcc-11e7-9af5-9734dd722ced.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458486-8b3cdcf8-edcc-11e7-974b-25c6085fa674.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458492-d69a77be-edcc-11e7-896f-21212621dbee.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458490-b3a1139e-edcc-11e7-88c8-79d4430d1514.png width=300> | facebook/react-native-website#105 - this docs PR is edited slightly from what's in `TextStylePropTypes` here; happy to align either one to the other after a review. [ANDROID] [FEATURE] [Text] - Implemented letterSpacing Closes #17398 Reviewed By: mdvacca Differential Revision: D6837718 Pulled By: hramos fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c
|
facebook/react-native#17398 has now been merged: facebook/react-native@5898817 |
vincentriemer
pushed a commit
to vincentriemer/react-native-dom
that referenced
this pull request
Mar 6, 2018
Summary: `letterSpacing` is completely missing from RN Android at the moment. I've reviewed the `letterSpacing` implementations in #13199, #13877 and #16801 (that all seem to have stalled) and managed to put together an improved one based on #13199, updated to merge cleanly post facebook/react-native@6114f86, that resolves the [issues](facebook/react-native#13199 (comment)) I've identified with that code. I believe this is the closest PR yet to a correct implementation of this feature, with a few caveats: - As with the other PRs, this only works on Android >= 5.0 (silently falling back to no letter spacing on older versions). Is this acceptable for a RN feature, in general? Would a dev mode warning be desirable? - The other PRs seem to have explored the space of potential solutions to the layout issue ([Android renders space _around_ glyphs](https://issuetracker.google.com/issues/37079859), iOS to the _right_ of each one) and come up empty, so I've opted to merely document the difference. - I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook/react-native#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers. - The implementation in `ReactEditText` is only there to handle the placeholder text, as `ReactBaseTextShadowNode` already affects the input control's contents correctly. - I'm not sure whether `<TextInput>` is meant to respect `allowFontScaling`; I've taken my cue here from `ReactTextInputManager.setFontSize()`, and used the same units (SP) to interpret the value in `ReactEditText.setLetterSpacingPt()`. - I'm not sure whether `<TextInput>` is even meant to support `letterSpacing` - it doesn't actually work on iOS. I'm not going to be able to handle the Objective-C side of this, not as part of this PR at least. - I have not added unit tests to `ReactTextTest` - is this desirable? I see that some other props such as `lineHeight` aren't covered there (unless I'm not looking in the right place). - Overall, I'm new to this codebase, so it's likely I've missed something not mentioned here. Note comment re: unit tests above; RNTester screenshots follow. | iOS (existing functionality, amended test) | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458459-c8d59498-edcb-11e7-8c8f-e7426f723886.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458473-2a1ca368-edcc-11e7-9ce6-30c6d3a48660.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458481-6c60a36e-edcc-11e7-9af5-9734dd722ced.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458486-8b3cdcf8-edcc-11e7-974b-25c6085fa674.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458492-d69a77be-edcc-11e7-896f-21212621dbee.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458490-b3a1139e-edcc-11e7-88c8-79d4430d1514.png width=300> | facebook/react-native-website#105 - this docs PR is edited slightly from what's in `TextStylePropTypes` here; happy to align either one to the other after a review. [ANDROID] [FEATURE] [Text] - Implemented letterSpacing Closes facebook/react-native#17398 Reviewed By: mdvacca Differential Revision: D6837718 Pulled By: hramos fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c
cpirich
pushed a commit
to tracemeinc/react-native
that referenced
this pull request
Mar 16, 2018
Summary: `letterSpacing` is completely missing from RN Android at the moment. I've reviewed the `letterSpacing` implementations in facebook#13199, facebook#13877 and facebook#16801 (that all seem to have stalled) and managed to put together an improved one based on facebook#13199, updated to merge cleanly post facebook@6114f86, that resolves the [issues](facebook#13199 (comment)) I've identified with that code. I believe this is the closest PR yet to a correct implementation of this feature, with a few caveats: - As with the other PRs, this only works on Android >= 5.0 (silently falling back to no letter spacing on older versions). Is this acceptable for a RN feature, in general? Would a dev mode warning be desirable? - The other PRs seem to have explored the space of potential solutions to the layout issue ([Android renders space _around_ glyphs](https://issuetracker.google.com/issues/37079859), iOS to the _right_ of each one) and come up empty, so I've opted to merely document the difference. - I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers. - The implementation in `ReactEditText` is only there to handle the placeholder text, as `ReactBaseTextShadowNode` already affects the input control's contents correctly. - I'm not sure whether `<TextInput>` is meant to respect `allowFontScaling`; I've taken my cue here from `ReactTextInputManager.setFontSize()`, and used the same units (SP) to interpret the value in `ReactEditText.setLetterSpacingPt()`. - I'm not sure whether `<TextInput>` is even meant to support `letterSpacing` - it doesn't actually work on iOS. I'm not going to be able to handle the Objective-C side of this, not as part of this PR at least. - I have not added unit tests to `ReactTextTest` - is this desirable? I see that some other props such as `lineHeight` aren't covered there (unless I'm not looking in the right place). - Overall, I'm new to this codebase, so it's likely I've missed something not mentioned here. Note comment re: unit tests above; RNTester screenshots follow. | iOS (existing functionality, amended test) | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458459-c8d59498-edcb-11e7-8c8f-e7426f723886.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458473-2a1ca368-edcc-11e7-9ce6-30c6d3a48660.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458481-6c60a36e-edcc-11e7-9af5-9734dd722ced.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458486-8b3cdcf8-edcc-11e7-974b-25c6085fa674.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458492-d69a77be-edcc-11e7-896f-21212621dbee.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458490-b3a1139e-edcc-11e7-88c8-79d4430d1514.png width=300> | facebook/react-native-website#105 - this docs PR is edited slightly from what's in `TextStylePropTypes` here; happy to align either one to the other after a review. [ANDROID] [FEATURE] [Text] - Implemented letterSpacing Closes facebook#17398 Reviewed By: mdvacca Differential Revision: D6837718 Pulled By: hramos fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c
guyca
pushed a commit
to wix-playground/react-native
that referenced
this pull request
Mar 29, 2018
Summary: `letterSpacing` is completely missing from RN Android at the moment. I've reviewed the `letterSpacing` implementations in facebook#13199, facebook#13877 and facebook#16801 (that all seem to have stalled) and managed to put together an improved one based on facebook#13199, updated to merge cleanly post facebook@6114f86, that resolves the [issues](facebook#13199 (comment)) I've identified with that code. I believe this is the closest PR yet to a correct implementation of this feature, with a few caveats: - As with the other PRs, this only works on Android >= 5.0 (silently falling back to no letter spacing on older versions). Is this acceptable for a RN feature, in general? Would a dev mode warning be desirable? - The other PRs seem to have explored the space of potential solutions to the layout issue ([Android renders space _around_ glyphs](https://issuetracker.google.com/issues/37079859), iOS to the _right_ of each one) and come up empty, so I've opted to merely document the difference. - I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers. - The implementation in `ReactEditText` is only there to handle the placeholder text, as `ReactBaseTextShadowNode` already affects the input control's contents correctly. - I'm not sure whether `<TextInput>` is meant to respect `allowFontScaling`; I've taken my cue here from `ReactTextInputManager.setFontSize()`, and used the same units (SP) to interpret the value in `ReactEditText.setLetterSpacingPt()`. - I'm not sure whether `<TextInput>` is even meant to support `letterSpacing` - it doesn't actually work on iOS. I'm not going to be able to handle the Objective-C side of this, not as part of this PR at least. - I have not added unit tests to `ReactTextTest` - is this desirable? I see that some other props such as `lineHeight` aren't covered there (unless I'm not looking in the right place). - Overall, I'm new to this codebase, so it's likely I've missed something not mentioned here. Note comment re: unit tests above; RNTester screenshots follow. | iOS (existing functionality, amended test) | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458459-c8d59498-edcb-11e7-8c8f-e7426f723886.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458473-2a1ca368-edcc-11e7-9ce6-30c6d3a48660.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458481-6c60a36e-edcc-11e7-9af5-9734dd722ced.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458486-8b3cdcf8-edcc-11e7-974b-25c6085fa674.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458492-d69a77be-edcc-11e7-896f-21212621dbee.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458490-b3a1139e-edcc-11e7-88c8-79d4430d1514.png width=300> | facebook/react-native-website#105 - this docs PR is edited slightly from what's in `TextStylePropTypes` here; happy to align either one to the other after a review. [ANDROID] [FEATURE] [Text] - Implemented letterSpacing Closes facebook#17398 Reviewed By: mdvacca Differential Revision: D6837718 Pulled By: hramos fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To go along with facebook/react-native#17398