-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Convert typography to use rem
units with px
fallbacks
#2302
Conversation
🦋 Changeset detectedLatest commit: f5cfde9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
} | ||
} | ||
|
||
// Text styles | ||
/* Set the font weight to normal */ | ||
.text-normal { font-weight: $font-weight-normal !important; } | ||
.text-normal { | ||
font-weight: $font-weight-normal !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind if it's a formatting fix, but wanted to double check you didn't mean to also add CSS vars for these lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually added CSS vars to these SCSS vars instead. It seemed like a safe change to make since no math would occur here 😄
// Font weights
$font-weight-bold: var(--base-text-weight-medium, 600) !default;
$font-weight-semibold: var(--base-text-weight-semibold, 500) !default;
$font-weight-normal: var(--base-text-weight-normal, 400) !default;
$font-weight-light: var(--base-text-weight-light, 300) !default;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
src/primitives/index.scss
Outdated
--h2-size-mobile: 1.375rem; | ||
--h3-size-mobile: 1.125rem; | ||
|
||
// Heading sizes - desktop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is off topic, but was wondering how we could replace:
--h00-size: 3rem;
--h2-size: 1.5rem;
since they don't exist for in the new typography tokens. I guess we could do:
--h00-size: 3rem;
->2.5rem
(--primer-text-display-size
)--h2-size: 1.5rem;
->1.25rem
(--primer-text-title-size-medium
)
And to make sure h2
and h3
will not look the same, use --primer-text-subtitle-size
(not bold) for h3
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On topic for sure! Basically, the new typography tokens were designed to be mapped to a Heading and Text component. We might be able to eventually reconcile our utilities to better map to them, but for now we want to just get these existing values into rem
units. So this may or may not end up being temporary..
…nto test-rem-typography
Hold off on merging this till after Universe. |
What are you trying to accomplish?
Safely test rem units for all typography base and utility styles behind a feature flag. Each change is using a CSS var that only exists behind a feature flag, with a fallback to the current value (scss var). There should be no visual changes to typography styles with this change.
Closes https://github.com/github/primer/issues/41
What approach did you choose and why?
CSS vars with fallbacks as this has been a useful method for testing our new set of design tokens.
What should reviewers focus on?
Can these changes ship as is?