-
Notifications
You must be signed in to change notification settings - Fork 1.2k
🚧 Replace font-size() and line-height() with custom properties #4674
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
Conversation
…size to browser default
| html { | ||
| position: relative; | ||
| font-size: ($base-font-size / $default-browser-font-size) * 100%; | ||
| font-size: 100%; |
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.
Is this the right time to introduce this change? It does impact the rem value we would set the font-size and line-height tokens to.
For context, there has been a previous attempt to make this change, but it was reverted #1590
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.
Might be worth getting @martenbjork to weigh in on this. I know he's experimented with global font size changes in the past
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.
My gut feeling is we tackle this when we remove the rem function. Would love to know other thoughts.
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.
@alex-page ok probably best not to conflate things. it's simple enough to update the token values when we do make the change.
size-limit report
|
| @return $value; | ||
| } @else if $unit == 'px' { | ||
| @return $value / $base-font-size * 1rem; | ||
| @return $value / $default-browser-font-size * 1rem; |
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 change is related to changing the base font size here https://github.com/Shopify/polaris-react/pull/4674/files#r752497881
| } | ||
| } | ||
|
|
||
| @mixin text-style-caption { |
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.
Leaving these mixins in for the scope of this PR. It would make sense to address them when we start thinking about composite components.
|
|
||
| .Text { | ||
| font-size: font-size(subheading); | ||
| font-size: var(--fs-1); |
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.
Glad to see we settled on token scales for font-size and line-height!
suggestion(non-blocking): Wonder if we should expand these abbreviations to match the --p-font-weight tokens @lgriffee just introduced? I know this is still WIP and the scope of this work is to get 100% token coverage, but I feel like we should have consistency in our typography tokens.
/* This doesn't feel very intuitive */
p {
font-size: var(--p-fs-1);
font-weight: var(--p-font-weight-regular)
}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 feel too strongly either way. Now is the time to play around with this and it will be easy enough to change through the token generator. Would it be worth creating a separate issue to align on naming convention?
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 think a separate issue is necessary personally. If anything I would just have a quick sync with @lgriffee to settle on a convention for v8. Right now it is clear two developers worked on the typography vars. I'm leaning towards expanding since fs isn't clear to me. If I was looking at a collection of :root properties does fs mean: flex-shrink, font-style, font-stretch, font-synthesis, font-size, font-size-adjustment, etc...?
WHY are these changes introduced?
Resolves #4600
WHAT is this pull request doing?
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes