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

Explicitly vertically center iOS text if IOSLineHeight is set #8851

Closed

Conversation

tuckerconnelly
Copy link
Contributor

This is another go at #7603

Made the lineHeight centering explicit by adding a new prop, IOSLineHeight, similar to a vendor prefix. Not thrilled about the name, but something like verticallyCenteredLineHeight felt off :P

Test plan:

Added another entry in the Text section of UIExplorer, and updated the screenshots.

Change baseline formatting for PR

Handle unset fontSizes in baseline calculation

Put baseline comment on the right line
@ghost
Copy link

ghost commented Jul 17, 2016

By analyzing the blame information on this pull request, we identified @grabbou and @rigdern 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 Jul 17, 2016
@satya164
Copy link
Contributor

cc @javache

@satya164
Copy link
Contributor

satya164 commented Jul 17, 2016

Why IOSLineHeight? Why not have something like verticalAlign: 'baseline' | 'middle'. Then center it if lineHeight is specified and verticalAlign is middle. (#1774)

Adding it as a variant of line height is confusing IMO. And it's not obvious what it does.

Not sure about making it default. Though I think this should be default, this is different than what's there on web, which can be confusing. Also as @bestander said, it breaks a lot of apps #7603 (comment)

To add, Android renders text at top by default I think, we need to fix that also.

cc @bestander @dmmiller

@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 Jul 17, 2016
@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 Jul 17, 2016
@dmmiller
Copy link

Agree with @satya164. LineHeight and alignment are two different things. Conflating them is wrong.

@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 Jul 18, 2016
@tuckerconnelly
Copy link
Contributor Author

tuckerconnelly commented Jul 18, 2016

Yeah I agree, and vertical-align is a good idea. Trying to get platform parity with web and android so I can share more code between the three.

Problem is vertical-align: baseline isn't the same as vertical-align: middle:

http://www.w3schools.com/cssref/playit.asp?filename=playcss_vertical-align&preval=baseline

Might stop pounding this and just maintain my own fork :P

@satya164
Copy link
Contributor

satya164 commented Jul 18, 2016

@tuckerconnelly

Problem is vertical-align: baseline isn't the same as vertical-align: middle:

The default is bottom on iOS. middle is what you wanna add support for, if I understand correctly.

I think it's useful to have verticalAlign in the core. And we get closer to the web in terms of supported properties.

@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 Jul 18, 2016
@tuckerconnelly
Copy link
Contributor Author

tuckerconnelly commented Jul 18, 2016

@satya164 Gotcha 😄 still playing around with vertical-align to learn it's nuances, sorry if it's obvious 😃

Seems like vertical-align only applies to children within a parent.

What web seems to do is vertically center text of the parent when a large line-height is set, regardless of what vertical-align is set. iOS aligns it to the bottom, even when it's the parent.

https://jsfiddle.net/7ymp3j8b/

^ If that were run in iOS, the first three examples would be aligned to the bottom. Implementing vertical-align, as it is on web, wouldn't fix this issue. Hence, why I chose vendor prefix (or breaking change) 😛

@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 Jul 18, 2016
@satya164
Copy link
Contributor

satya164 commented Jul 18, 2016

@tuckerconnelly

If we're breaking lots of apps by changing the current RN behaviour, may be we just introduce the feature, and keep the current behaviour as default for now.

So, may be just add the values possible right now (how about 'top' | 'bottom' | 'middle' ?), and keep the default value as bottom? baseline is a little different from middle as it aligns the lowest hanging parts of the letters.

On the web we can apply this to any inline, inline-block or table-cells, but RN doesn't have these, and everything is flexbox. So we only have text to which we can apply this.

We shouldn't add an IOS prefix to this, coz we eventually want it on Android too. If we can't get the exact behaviour as on the web since we break lots of apps, I think we should compromise, and keep the current behaviour.

@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 Jul 18, 2016
@devKrishnan
Copy link

I was able to set the text vertically middle aligned by setting the the below values to the style of the container view.

 flex: 1,
    flexDirection: 'row',
    alignItems:'center'

@tuckerconnelly
Copy link
Contributor Author

Pretty sure that only works with a single line :)

@satya164
Copy link
Contributor

satya164 commented Aug 4, 2016

There's a textAlignVertical prop supported on Android - http://facebook.github.io/react-native/docs/text.html

@ghost ghost closed this in 68d483e Aug 11, 2016
@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 12, 2016
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
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
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
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants