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

[iOS] Add ultrabold pairs for font weight #24948

Closed
wants to merge 1 commit into from

Conversation

zhongwuzw
Copy link
Contributor

Summary

Add ultrabold map of font weight, if not, ultrabold would map to bold.
Fixes #23512.

Changelog

[iOS] [Fixed] - Add ultrabold pairs for font weight

Test Plan

After:

<Text style={{fontFamily: 'GillSans-UltraBold'}}>Hello</Text>
image

<Text style={{fontFamily: 'GillSans-Bold'}}>Hello</Text>
image

Before:

<Text style={{fontFamily: 'GillSans-UltraBold'}}>Hello</Text>
image

<Text style={{fontFamily: 'GillSans-Bold'}}>Hello</Text>
image

@zhongwuzw zhongwuzw requested a review from shergin as a code owner May 19, 2019 07:59
@zhongwuzw zhongwuzw requested a review from cpojer May 19, 2019 08:00
@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 May 19, 2019
@react-native-bot react-native-bot added Platform: iOS iOS applications. Bug labels May 19, 2019
@shergin
Copy link
Contributor

shergin commented May 19, 2019

Just curious, does it work in Fabric (without the fix)?
In Fabric we use:

static UIFontWeight RCTGetFontWeight(UIFont *font) {
  NSDictionary *traits =
      [font.fontDescriptor objectForKey:UIFontDescriptorTraitsAttribute];
  return [traits[UIFontWeightTrait] doubleValue];
}

Does it sufficient? Why should we parse the font name in the first place?

@zhongwuzw
Copy link
Contributor Author

Just curious, does it work in Fabric (without the fix)?

🤔 After we fixes #24966, it works in this case.

In Fabric we use:

static UIFontWeight RCTGetFontWeight(UIFont *font) {
  NSDictionary *traits =
      [font.fontDescriptor objectForKey:UIFontDescriptorTraitsAttribute];
  return [traits[UIFontWeightTrait] doubleValue];
}

Does it sufficient? Why should we parse the font name in the first place?

From the discussion in #15162, seems it's not sufficient, because there has inconsistency for some fonts to parse weight, so we parse the font name in the first place. Should we also add that in Fabric? 🤔

@shergin
Copy link
Contributor

shergin commented May 20, 2019

inconsistency for some fonts to parse weight

Do you mean inconsistency between how iOS interprets the fonts and how classic RN does it? Or which kind of inconsistency?

@zhongwuzw
Copy link
Contributor Author

@shergin Yeah, based on #15162, they said Roboto weight 200/300 both resolve to thin, but they should resolve to thin and light respectively.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Let's ship it.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @zhongwuzw in 9d0d7b6.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 31, 2019
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
Add `ultrabold` map of font weight, if not, `ultrabold` would map to `bold`.
Fixes facebook#23512.

## Changelog

[iOS] [Fixed] - Add ultrabold pairs for font weight
Pull Request resolved: facebook#24948

Differential Revision: D15575568

Pulled By: cpojer

fbshipit-source-id: 5d1d6a033c166d91a330526ba8996ac0416f3887
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RCTFont maps UltraBold font to UIFontWeightBold font weight
5 participants