Skip to content

[iOS] Add ultrabold pairs for font weight - #24948

Closed
zhongwuzw wants to merge 1 commit into
react:masterfrom
zhongwuzw:fix_font_weight
Closed

[iOS] Add ultrabold pairs for font weight#24948
zhongwuzw wants to merge 1 commit into
react:masterfrom
zhongwuzw:fix_font_weight

Conversation

@zhongwuzw

Copy link
Copy Markdown
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 ownerMay 19, 2019 07:59
@zhongwuzw
zhongwuzw requested a review from cpojerMay 19, 2019 08:00
@facebook-github-botfacebook-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
@shergin

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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

@cpojercpojer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's ship it.

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Collaborator

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

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

@react-native-botreact-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`.
Fixesreact#23512.
## Changelog
[iOS] [Fixed] - Add ultrabold pairs for font weight
Pull Request resolved: react#24948
Differential Revision: D15575568
Pulled By: cpojer
fbshipit-source-id: 5d1d6a033c166d91a330526ba8996ac0416f3887
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BugCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: iOSiOS applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RCTFont maps UltraBold font to UIFontWeightBold font weight

5 participants

@zhongwuzw@shergin@react-native-bot@cpojer@facebook-github-bot