Skip to content

Centre text in case where the distance between baselines is larger th… #9211

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.

Sign up for GitHub

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

Jump to bottom
Closed
jacobp100 wants to merge 6 commits into from
Closed

Centre text in case where the distance between baselines is larger th… #9211

jacobp100 wants to merge 6 commits into from

Conversation

@jacobp100
Copy link
Contributor

@jacobp100 jacobp100 commented Aug 4, 2016

…an the font size (per CSS spec)

Extending upon work #7603
Closes #8851

screen shot 2016-08-04 at 14 49 21
screen shot 2016-08-04 at 14 49 24

@ghost
Copy link

ghost commented Aug 4, 2016

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

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 4, 2016
ide
ide reviewed Aug 4, 2016
View reviewed changes
Libraries/Text/RCTShadowText.m Outdated

if (lineHeight > fontSize) {
[attributedString addAttribute:NSBaselineOffsetAttributeName
value:@(lineHeight/2 - fontSize/2)
Copy link
Contributor

@ide ide Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Spaces around operators (/)

@ide
Copy link
Contributor

ide commented Aug 4, 2016

I'm not super familiar with typography and am unsure if (lineHeight - fontSize) / 2 is the right calculation for the offset. In CSS at least, fontSize apparently is not directly related to the height and MDN says: The font-size CSS property specifies the size of the font (historically the width of the capital "M"). I believe we want a calculation closer to (lineHeightWeAreGoingToUse - lineHeightIntrinsicToThisText) / 2.

Fix font centering---use the font line height over the font point siz…
842e4b8 
…e to get the actual space the font takes up
@ghost
Copy link

ghost commented Aug 5, 2016

@jacobp100 updated the pull request.

@jacobp100
Copy link
Contributor Author

jacobp100 commented Aug 5, 2016

You’re correct in that. I’ve updated it to use the font lineHeight property.

@ghost 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 5, 2016
@ide
Copy link
Contributor

ide commented Aug 5, 2016

Looks like the Obj-C tests are failing (UIExplorerSnapshotTests in https://travis-ci.org/facebook/react-native/jobs/149979798) -- some of the tests capture screenshots and do a before-and-after comparison, so you probably need to re-record them.

See https://facebook.github.io/react-native/releases/0.31/docs/testing.html#snapshot-tests-ios for more about the screenshot tests.

@ghost 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 5, 2016
@ghost
Copy link

ghost commented Aug 5, 2016

@jacobp100 updated the pull request.

@ghost 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 5, 2016
@ghost
Copy link

ghost commented Aug 5, 2016

@jacobp100 updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Aug 5, 2016

@jacobp100 updated the pull request.

@ghost
Copy link

ghost commented Aug 5, 2016

@jacobp100 updated the pull request.

@ghost 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 5, 2016
@ide
Copy link
Contributor

ide commented Aug 11, 2016

@facebook-github-bot shipit

@ghost ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 11, 2016
@ghost
Copy link

ghost commented Aug 11, 2016

Thanks for importing.If you are an FB employee go to Phabricator to review internal test results.

@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 Aug 11, 2016
@ghost ghost closed this in 68d483e Aug 11, 2016
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
@jacobp100
Centre text in case where the distance between baselines is larger th…
99041b8 
Summary:
…an the font size (per CSS spec)

Extending upon work facebook#7603
Closes facebook#8851

![screen shot 2016-08-04 at 14 49 21](https://cloud.githubusercontent.com/assets/7275322/17404165/bd67bc48-5a52-11e6-9ba1-5a8524f18867.png)
![screen shot 2016-08-04 at 14 49 24](https://cloud.githubusercontent.com/assets/7275322/17404167/be5f4044-5a52-11e6-9014-391349f9c5e1.png)
Closes facebook#9211

Differential Revision: D3706347

fbshipit-source-id: 0adfff8e8418b02f9b5d6671f5c89669e41abec3
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Aug 25, 2016
@jacobp100
Centre text in case where the distance between baselines is larger th…
16d89c1 
Summary:
…an the font size (per CSS spec)

Extending upon work facebook/react-native#7603
Closes facebook/react-native#8851

![screen shot 2016-08-04 at 14 49 21](https://cloud.githubusercontent.com/assets/7275322/17404165/bd67bc48-5a52-11e6-9ba1-5a8524f18867.png)
![screen shot 2016-08-04 at 14 49 24](https://cloud.githubusercontent.com/assets/7275322/17404167/be5f4044-5a52-11e6-9014-391349f9c5e1.png)
Closes facebook/react-native#9211

Differential Revision: D3706347

fbshipit-source-id: 0adfff8e8418b02f9b5d6671f5c89669e41abec3
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jacobp100 @ide @facebook-github-bot