<Text> Expose Android's includeFontPadding property to JavaScript. #9323

benvium · 2016-08-09T17:50:27Z

By default Android will put extra space above text to allow for upper-case accents or other ascenders. With some fonts, this can make text look slightly misaligned when centered vertically.

We have found that the effect is very noticeable with certain custom fonts on Android. On iOS the font aligns vertically as expected.

Android exposes a property includeFontPadding that will remove this extra padding if set to false. This PR exposes that to JS, and adds it to the documentation and UIExplorer.

Test Plan:

Run UIExplorer
Scroll down to find the option
Click and scroll to bottom to see a sample with the default text vertical alignment (includeTextPadding: true) and the result when using includeTextPadding: false.

Add example to UIExplorer (android only) By default Android will put extra space above text to allow for upper-case accents or other ascenders. With some fonts, this can make text look slightly misaligned when centered vertically. Android OS supplies a useful property includeTextPadding that can alleviate this effect for some fonts. We have found that the effect is very noticeable with certain custom fonts on Android. On iOS the font aligns vertically as expected. Test Plan: - Run UIExplorer - Scroll down to find the <Text> option - Click and scroll to bottom to see a sample with the default text vertical alignment (includeTextPadding: true) and the result when using includeTextPadding: false.

ghost · 2016-08-09T17:52:04Z

By analyzing the blame information on this pull request, we identified @skv-headless and @spicyj to be potential reviewers.

ghost · 2016-08-09T17:52:06Z

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 [email protected]. Thanks!

facebook-github-bot · 2016-08-09T17:58:20Z

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

eslint-bot · 2016-08-09T18:00:00Z

Examples/UIExplorer/js/TextExample.android.js

@@ -411,6 +411,23 @@ class TextExample extends React.Component {
            This very long text should be truncated with dots in the beginning.
          </Text>
        </UIExplorerBlock>
+        <UIExplorerBlock title="Include Font Padding">
+          <View style={{flexDirection: "row", justifyContent: 'space-around', marginBottom: 10}}>


quotes: Strings must use singlequote.

ghost · 2016-08-09T18:06:37Z

@benvium updated the pull request.

rigdern · 2016-08-30T04:59:17Z

@spicyj, have you had a chance to review this yet? This feature would be useful in my team's app.

Adam Comella
Microsoft Corp.

sophiebits · 2016-08-30T05:04:55Z

@mkonicek Can you triage?

ghost · 2016-09-09T17:01:17Z

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @rigdern as a potential reviewer. Could you take a look please or cc someone with more context?

rigdern · 2016-09-09T19:18:04Z

cc @mkonicek

rigdern · 2016-10-10T21:40:21Z

ping @mkonicek, this feature would be useful in my team's app. Can somebody review it?

hramos · 2016-11-10T22:57:37Z

Triaging...

lacker · 2016-11-30T22:13:28Z

This looks good to me. I wish there were a better way to test this sort of thing but since it's just passing through a parameter this might be overkill.

lacker · 2016-11-30T22:14:01Z

@benvium Can you resolve the merge conflict and then I'll ship this?

benvium · 2016-11-30T22:41:49Z

@lacker Yes, I can. Will do tomorrow morning UK time. Thank you.

# Conflicts: # ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java

benvium · 2016-12-01T09:20:29Z

@lacker I have updated the PR as requested. I re-tested using UIExplorer on API 22 and confirmed that it still works.

lacker · 2016-12-01T16:43:53Z

The Travis failure seems spurious, let me try rerunning it.

benvium · 2016-12-02T11:56:48Z

@lacker Looks like it succeeded.

facebook-github-bot · 2016-12-02T19:06:59Z

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

Summary: By default Android will put extra space above text to allow for upper-case accents or other ascenders. With some fonts, this can make text look slightly misaligned when centered vertically. We have found that the effect is very noticeable with certain custom fonts on Android. On iOS the font aligns vertically as expected. Android exposes a property `includeFontPadding` that will remove this extra padding if set to false. This PR exposes that to JS, and adds it to the documentation and UIExplorer. Closes facebook#9323 Differential Revision: D4266713 Pulled By: lacker fbshipit-source-id: f9711254bc26c09b4586a865f0e95ef4bf77cf3f

Summary: By default Android will put extra space above text to allow for upper-case accents or other ascenders. With some fonts, this can make text look slightly misaligned when centered vertically. We have found that the effect is very noticeable with certain custom fonts on Android. On iOS the font aligns vertically as expected. Android exposes a property `includeFontPadding` that will remove this extra padding if set to false. This PR exposes that to JS, and adds it to the documentation and UIExplorer. Closes #9323 Differential Revision: D4266713 Pulled By: lacker fbshipit-source-id: f9711254bc26c09b4586a865f0e95ef4bf77cf3f

Summary: By default Android will put extra space above text to allow for upper-case accents or other ascenders. With some fonts, this can make text look slightly misaligned when centered vertically. We have found that the effect is very noticeable with certain custom fonts on Android. On iOS the font aligns vertically as expected. Android exposes a property `includeFontPadding` that will remove this extra padding if set to false. This PR exposes that to JS, and adds it to the documentation and UIExplorer. Closes facebook#9323 Differential Revision: D4266713 Pulled By: lacker fbshipit-source-id: f9711254bc26c09b4586a865f0e95ef4bf77cf3f

Summary: By default Android will put extra space above text to allow for upper-case accents or other ascenders. With some fonts, this can make text look slightly misaligned when centered vertically. We have found that the effect is very noticeable with certain custom fonts on Android. On iOS the font aligns vertically as expected. Android exposes a property `includeFontPadding` that will remove this extra padding if set to false. This PR exposes that to JS, and adds it to the documentation and UIExplorer. Closes facebook/react-native#9323 Differential Revision: D4266713 Pulled By: lacker fbshipit-source-id: f9711254bc26c09b4586a865f0e95ef4bf77cf3f

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

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 9, 2016

eslint-bot reviewed Aug 9, 2016
View reviewed changes

Fix issues identified by ESLint

08bf092

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 Aug 9, 2016

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 30, 2016

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 9, 2016

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 Oct 10, 2016

hramos added the Core Team label Nov 10, 2016

lacker self-assigned this Nov 30, 2016

Merge branch 'master' into android-include-font-padding-style

62809d6

# Conflicts: # ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Dec 2, 2016

facebook-github-bot closed this in 833961e Dec 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

<Text> Expose Android's includeFontPadding property to JavaScript. #9323

<Text> Expose Android's includeFontPadding property to JavaScript. #9323

benvium commented Aug 9, 2016

ghost commented Aug 9, 2016

ghost commented Aug 9, 2016

facebook-github-bot commented Aug 9, 2016

eslint-bot Aug 9, 2016

ghost commented Aug 9, 2016

rigdern commented Aug 30, 2016

sophiebits commented Aug 30, 2016

ghost commented Sep 9, 2016

rigdern commented Sep 9, 2016

rigdern commented Oct 10, 2016

hramos commented Nov 10, 2016

lacker commented Nov 30, 2016

lacker commented Nov 30, 2016

benvium commented Nov 30, 2016

benvium commented Dec 1, 2016

lacker commented Dec 1, 2016

benvium commented Dec 2, 2016

facebook-github-bot commented Dec 2, 2016

<Text> Expose Android's includeFontPadding property to JavaScript. #9323

<Text> Expose Android's includeFontPadding property to JavaScript. #9323

Conversation

benvium commented Aug 9, 2016

ghost commented Aug 9, 2016

ghost commented Aug 9, 2016

facebook-github-bot commented Aug 9, 2016

eslint-bot Aug 9, 2016

Choose a reason for hiding this comment

ghost commented Aug 9, 2016

rigdern commented Aug 30, 2016

sophiebits commented Aug 30, 2016

ghost commented Sep 9, 2016

rigdern commented Sep 9, 2016

rigdern commented Oct 10, 2016

hramos commented Nov 10, 2016

lacker commented Nov 30, 2016

lacker commented Nov 30, 2016

benvium commented Nov 30, 2016

benvium commented Dec 1, 2016

lacker commented Dec 1, 2016

benvium commented Dec 2, 2016

facebook-github-bot commented Dec 2, 2016