Add typography variables #482
Conversation
…to css/hadas/typography � Conflicts: � package-lock.json � package.json � src/components/AttentionBox/AttentionBox.scss
| line-height: $line-height-for-size-18; | ||
| font-family: var(--font-family); | ||
| font-weight: var(--font-weight-normal); | ||
| line-height: 21px; |
There was a problem hiding this comment.
we usually put different padding for 18px and therefore removed the variable
| @@ -8,7 +8,6 @@ | |||
| outline: none; | |||
| border: none; | |||
| height: auto; | |||
| font-family: Roboto, sans-serif; | |||
There was a problem hiding this comment.
because we use mixins for fonts here for each button size which include font family setting
| @@ -3,6 +3,9 @@ | |||
| $avatar-icon-large: 28px; | |||
| $avatar-icon-medium: 18px; | |||
| $avatar-icon-small: 12px; | |||
| $font-size-avatar-large: 18px; | |||
There was a problem hiding this comment.
moved out of typography because its not the default sizes which in css variables now
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| // font family | |||
| $header-primary-font: "Sofia Pro", Roboto, sans-serif; | |||
| $text-primary-font: Roboto, sans-serif; | |||
| $text-primary-font: var(--font-family); | |||
There was a problem hiding this comment.
its ok because we don't want our storybook to be depended at the system font
hadasfa
changed the title
| @@ -74,9 +74,9 @@ export const TextStyles = () => { | |||
| </VisualDescription> | |||
| <VisualDescription | |||
| ariaLabel="H5" | |||
| title="Fourth heading (Roboto 18px bold)" | |||
| title="Fourth heading (Roboto 16px bold)" | |||
There was a problem hiding this comment.
(might be out of this PR's scope) looks like the details can easily get out of sync with the actual rules (meaning, the rules might change to be Roboto 16px semi-bold, but no one will remember to update this story).
Is there a way to automatically sync between the rules and the stories? perhaps exporting a SASS variable?
There was a problem hiding this comment.
We exporting the css variables for this exactly. ׳e need to add them to this doc and i will talk with eivgany about the design of the doc with the variables.
src/styles/typography.scss
Outdated
| @@ -29,129 +9,81 @@ $p-empty-state-height: 16px; | |||
| $h6-empty-state-height: 12px; | |||
| $small-empty-state-height: 12px; | |||
|
|
|||
| @mixin base-font { | |||
There was a problem hiding this comment.
looks like the usage of base-font always comes along with a font rule. What do you think about adding the font rule as an argument to base-font, so that future devs will never forget that this is required?
There was a problem hiding this comment.
Agreed, will be fixed
…to css/hadas/typography
…to css/hadas/typography � Conflicts: � package-lock.json � package.json
|
Your PR should include one (and only one) of the following labels:
|
Basic
npm run plop) to create a new component.PropTypes.Style
NewComponent.modules.scssfile inside of theNewComponentfolder.Storybook
/src/NewComponent/__stories__/NewComponent.stories.jsfile.Tests