-
Notifications
You must be signed in to change notification settings - Fork 68
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
refactor(core, normalize): add variables for new type style families and remove ligatures in normalize #1543
Conversation
@@ -13,6 +13,74 @@ | |||
--psTypeFontSizeSmall: 14px; | |||
--psTypeFontSizeXSmall: 12px; |
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.
Do you think the old font sizes, weights and line heights should be removed? If not, why keep?
@@ -1,7 +1,7 @@ | |||
export default { | |||
fontFamily: | |||
'"PS TT Commons Roman", "Gotham SSm A", "Gotham SSm B", Arial, sans-serif', | |||
fontFamilyCode: '"Source Code Pro", monospace', | |||
fontFamilyCode: '"DM Mono", monospace', |
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 probably needs updated in the docs too.
@@ -25,7 +25,7 @@ exports[`Storyshots drawer controlled: external state 1`] = ` | |||
role="button" | |||
> | |||
<p | |||
data-css-r3ja7m="" | |||
data-css-1joltl8="" |
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.
Can you speak to the reason that the snapshots in this pr have changed? As I understand it now, they shouldn't have changed because no styles/values have changed, only the variables used to represent those values.
lineHeight: '16px', | ||
textTransform: 'uppercase', | ||
letterSpacing: '.08em' | ||
fontSize: type.headingXXXSmallFontSize, |
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.
👍
@@ -17,7 +17,7 @@ html { | |||
font-size: 0.875em; | |||
font-weight: var(--psTypeFontWeightRegular); | |||
line-height: 1.71429; /* calc(var(--psTypeLineHeightStandard) / var(--psTypeFontSizeSmall)); */ | |||
font-feature-settings: 'tnum' on, 'lnum' on; | |||
font-feature-settings: 'tnum' on, 'lnum' on, 'liga' off; |
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 seems to be the only thing I can find that's a new style. Correct?
Even if that's the case, I don't see this influencing the data-css-hashes, because normalize is a peerDependency that's just expected to be on the page, added to the style cascade in the browser, not present at build time.
Team triage: pause for now. Rethink general vars in core. Make sure we can support teams in their usage patterns for the Text components. |
Closes #1547
core
normalize
text