Skip to content

(iOS) Support for DemiBold alias of SemiBold (font-weight 600) #15856

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
wants to merge 2 commits into from
Closed

(iOS) Support for DemiBold alias of SemiBold (font-weight 600) #15856

wants to merge 2 commits into from

Conversation

wildseansy
Copy link
Contributor

For more information on font weight naming see this post.

DemiBold is fairly common in font naming. For example, iOS React Native lacks support of "AvNext-DemiBold".

Also removed warning about NSUInteger <=> NSInteger comparison, by making i an NSUInteger

Before and after screenshots:

Before fix

before-fix

After

after-fix

Test Plan

Add any DemiBold font to an iOS react native project. Set fontWeight to '600' on a <Text /> component. The font weight should be applied appropriately.

@wildseansy
Support for DemiBold alias of SemiBold
a0a64d0 
For more information see [this post](https://www.quora.com/What-is-the-difference-between-Medium-Demi-and-Semibold-fonts).

DemiBold is fairly common in font naming, and React Native lacks support of "AvNext-DemiBold" for example.
@wildseansy wildseansy requested a review from shergin as a code owner September 7, 2017 20:00
@facebook-github-bot
Copy link
Contributor

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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@wildseansy wildseansy changed the title Support for DemiBold alias of SemiBold (font-weight 600) (iOS) Support for DemiBold alias of SemiBold (font-weight 600) Sep 7, 2017
@pull-bot
Copy link

pull-bot commented Sep 7, 2017 •
edited
Loading

Attention: @shergin

Generated by 🚫 dangerJS

@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 Sep 7, 2017
@facebook-github-bot
Copy link
Contributor

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

2 similar comments
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@ide
Copy link
Contributor

ide commented Sep 7, 2017

From what I can tell, these are valid aliases. Have you looked into the Android side as well?

@wildseansy
Copy link
Contributor Author

wildseansy commented Sep 7, 2017 •
edited
Loading

@ide - from what I have read, using numeric font weights ('500', '600', '700'...etc) don't work in android. Is this not the case? In Android, I currently just explicitly specify fontFamily for each weight

@wildseansy
Copy link
Contributor Author

@ide - here's a post that outlines the discrepancy between iOS and android when it comes to font weights. Because there is no font-weight-matching logic on android from a numeric value to a font name, this problem doesn't (yet) apply. On Android you have to be discrete about each font name, which means demibold will already work (it does for me testing both).

shergin
shergin suggested changes Sep 9, 2017
View reviewed changes
Copy link
Contributor

@shergin shergin left a comment

Choose a reason for hiding this comment

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

👍

React/Views/RCTFont.mm Outdated
@(UIFontWeightBold),
@(UIFontWeightHeavy),
@(UIFontWeightBlack)
];
});

for (NSInteger i = 0; i < fontNames.count; i++) {
for (NSUInteger i = 0; i < fontNames.count; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The modern Apple convention is "always use NSInteger unless you really need to use NSUInteger", we are following it in RN.

@facebook-github-bot 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 Sep 9, 2017
@facebook-github-bot
Copy link
Contributor

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

Omniwheel pushed a commit to Root-App/react-native that referenced this pull request Oct 25, 2017
@wildseansy @Omniwheel
(iOS) Support for DemiBold alias of SemiBold (font-weight 600)
f867fca 
Summary:
For more information on font weight naming see [this post](https://www.quora.com/What-is-the-difference-between-Medium-Demi-and-Semibold-fonts).

DemiBold is fairly common in font naming. For example, iOS React Native lacks support of "AvNext-DemiBold".

Also removed warning about `NSUInteger` <=> `NSInteger` comparison, by making `i` an `NSUInteger`

Before and after screenshots:

![before-fix](https://user-images.githubusercontent.com/177857/30182567-1dfebcb0-93cc-11e7-9b51-78ef6f41c447.png)

![after-fix](https://user-images.githubusercontent.com/177857/30182570-1f90ea94-93cc-11e7-8f68-008ae648ffbe.png)

Add any DemiBold font to an iOS react native project.  Set fontWeight to `'600'` on a `<Text />` component. The font weight should be applied appropriately.
Closes facebook#15856

Differential Revision: D5800928

Pulled By: shergin

fbshipit-source-id: 9095e3e150847f9cb828aa5d080567846441e55d
Omniwheel pushed a commit to Root-App/react-native that referenced this pull request Nov 2, 2017
@wildseansy @Omniwheel
(iOS) Support for DemiBold alias of SemiBold (font-weight 600)
53c82b5 
Summary:
For more information on font weight naming see [this post](https://www.quora.com/What-is-the-difference-between-Medium-Demi-and-Semibold-fonts).

DemiBold is fairly common in font naming. For example, iOS React Native lacks support of "AvNext-DemiBold".

Also removed warning about `NSUInteger` <=> `NSInteger` comparison, by making `i` an `NSUInteger`

Before and after screenshots:

![before-fix](https://user-images.githubusercontent.com/177857/30182567-1dfebcb0-93cc-11e7-9b51-78ef6f41c447.png)

![after-fix](https://user-images.githubusercontent.com/177857/30182570-1f90ea94-93cc-11e7-8f68-008ae648ffbe.png)

Add any DemiBold font to an iOS react native project.  Set fontWeight to `'600'` on a `<Text />` component. The font weight should be applied appropriately.
Closes facebook#15856

Differential Revision: D5800928

Pulled By: shergin

fbshipit-source-id: 9095e3e150847f9cb828aa5d080567846441e55d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@shergin shergin shergin requested changes

Reviewers whose approvals may not affect merge requirements
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.
5 participants
@wildseansy @facebook-github-bot @pull-bot @ide @shergin