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

Add font-weight-medium to marketing styles, and use it in the type scale #1497

Merged
merged 9 commits into from
Jul 14, 2021

Conversation

tobiasahlin
Copy link
Contributor

@tobiasahlin tobiasahlin commented Jul 12, 2021

The medium weight of a font usually maps to 500, but this weight is taken in Primer by semibold. In order to add the ability to use medium weights on marketing pages, this PR adds medium at weight 450 and extrabold at 800, and adds a .text-medium utility to use this font-weight. It also changes the marketing type scale to use medium rather than semibold for all fX-mktg utilities.

This produces the full weight scale as follows:

$font-weight-extrabold: 800 !default;
$font-weight-bold: 600 !default;
$font-weight-semibold: 500 !default;
$font-weight-medium: 450 !default;
$font-weight-normal: 400 !default;
$font-weight-light: 300 !default;

@changeset-bot
Copy link

changeset-bot bot commented Jul 12, 2021

🦋 Changeset detected

Latest commit: 99828a1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ajashams
Copy link

@trosage @tobiasahlin -- I'm just comparing these across figma.

Figma Suggested code updates
Screen Shot 2021-07-12 at 12 10 53 PM Screen Shot 2021-07-12 at 12 11 04 PM $font-weight-extrabold: 700 !default;
$font-weight-bold: 600 !default;
$font-weight-semibold: 500 !default;
$font-weight-medium: 450 !default;
$font-weight-normal: 400 !default;
$font-weight-light: 300 !default;

There are a few discrepancies so i want to make sure I'm up to speed on any other changes we may have made prior to this convo beforehand.

@tobiasahlin tobiasahlin requested a review from simurai July 13, 2021 07:56
@tobiasahlin
Copy link
Contributor Author

tobiasahlin commented Jul 13, 2021

@ajashams All of the numbers in this implementation are just semantic and are used to map a certain number/weight to a font file 🙏 so the numbers differ from Figma but none of the actual used font weights in the type scale should differ in the end!

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

Should the class and variables names be pre/suffixed with mktg?

So that it's clearer that this is only available in marketing and not in core?

@tobiasahlin
Copy link
Contributor Author

tobiasahlin commented Jul 13, 2021

@simurai I've been battling with this choice internally 😅 adding mktg for buttons for example makes total sense, when we have two flavors of a similar component. But in a way I think this addition is more akin to when we extended the padding and margin scales, which was only available in the marketing bundle but kept the naming scheme for simplicity. I think this makes sense from the perspective that otherwise using the font weight scale would require you to go back and forth between appending mktg and not (mktg-font-medium, font-semibold, font-bold, mktg-font-extrabold...), which could be a bit confusing.

For what it's worth, these utilities also don't use mktg yet are specific to the marketing bundle:

.width-auto {}
.height-auto {}

// Make an object fill its parent
.object-fit-cover {}

// Handling z-index
.z-1 {}
.z-2 {}
.z-3 {}

// Negative z-index
.z-n1 {}
.z-n2 {}

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

when we have two flavors of a similar component
more akin to when we extended

Ahh, I see. Using mktg is more for something unique or an alternative. But feels weird when just extending core stuff. Ok, yeah.. that sounds reasonable to me. 👍

@tobiasahlin
Copy link
Contributor Author

Bumped the extra-bold weight to 800 to reduce flickering at font swap: the --apple-system font (San Francisco) is much closer to Alliance Extra Bold at weight 800 🙏

@tobiasahlin tobiasahlin merged commit 84bbd50 into main Jul 14, 2021
@tobiasahlin tobiasahlin deleted the tobiasahlin/add-medium-to-mktg-typescale branch July 14, 2021 14:18
@primer-css primer-css mentioned this pull request Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants