Skip to content
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.

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

Vertically center iOS text if lineHeight is set #7603

Conversation

tuckerconnelly
Copy link
Contributor

First PR!!

This fixes #2991 :)

@ghost
Copy link

ghost commented May 17, 2016

By analyzing the blame information on this pull request, we identified @nicklockwood and @Kudo 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 May 17, 2016
@charliesbot
Copy link

this is really helpful! please approve this pr

@mkonicek
Copy link
Contributor

@tuckerconnelly Can you provide some screenshots from UIExplorer before and after? This might change how existing apps look so it is likely a breaking change though.

@nicklockwood, @javache What do you think?

// vertically center text
CGFloat fontSize = round(_fontSize * (_allowFontScaling && self.fontSizeMultiplier > 0.0 ? self.fontSizeMultiplier : 1.0));
[attributedString addAttribute:NSBaselineOffsetAttributeName
value:[NSNumber numberWithFloat:lineHeight/2 - fontSize/2]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can you replace

[NSNumber numberWithFloat:lineHeight/2 - fontSize/2]

With

@(lineHeight/2 - fontSize/2)

@nicklockwood
Copy link
Contributor

@mkonicek I agree that this should be the default behaviour. Difficult to guess what the impact might be on existing apps though.

@ghost
Copy link

ghost commented May 29, 2016

@tuckerconnelly updated the pull request.

@ghost
Copy link

ghost commented May 29, 2016

@tuckerconnelly updated the pull request.

@tuckerconnelly
Copy link
Contributor Author

Boom, before:

screen shot 2016-05-29 at 4 39 24 pm

after:

screen shot 2016-05-29 at 4 40 35 pm

Also added code for adjusting the baseline when fontSize wasn't set.

@nihgwu
Copy link
Contributor

nihgwu commented Jun 8, 2016

any update? i'm looking forward this pr to be merged

@nicklockwood
Copy link
Contributor

@facebook-github-bot shipit

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

ghost commented Jun 10, 2016

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

@ghost ghost closed this in 33dfc9d Jun 10, 2016
@bestander
Copy link
Contributor

Shame for shipping this without looking at Travis build status https://travis-ci.org/facebook/react-native/jobs/133786669

This PR broke trunk

@bestander
Copy link
Contributor

After talking to the teams internally, looks like we can't accept this change, have to revert it.
The problem is that it breaks the default behaviour and a lot of apps got broken.

@bestander bestander reopened this Jun 10, 2016
@bestander
Copy link
Contributor

Feel free to do another iteration and submit a new PR

@bestander bestander closed this Jun 10, 2016
ghost pushed a commit that referenced this pull request Jun 10, 2016
Summary:
First PR!!

This fixes #2991 :)
Closes #7603

Differential Revision: D3417257

Pulled By: bestander

fbshipit-source-id: fca990a973edaf7bae5481d03f5da5de0c085c16
@tuckerconnelly
Copy link
Contributor Author

That's the nature of a breaking change lol

What would you want in another iteration?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

[Style][iOS]The style lineHeight behaves not exactly same as CSS
6 participants