-
Notifications
You must be signed in to change notification settings - Fork 1
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
[FE] Typography component #78
[FE] Typography component #78
Conversation
className?: string; | ||
} | ||
|
||
export type TypographyVariants = |
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 was considering splitting variant and font weight to different parameters, but I decided to leave it this way to stay consistent with typography styles from Figma (I only shortened 'Heading' to 'head'). I think it would be easier to use while implementing new views:
For example:
Jane Edge has a style named "Headline M/Meddium", so for the Typography component we should use the head-m/medium
variant
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 agree, I've split button styles into two separate props to reflect Figma. Your approach makes sense to me 👌
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.
Good ideas, let's go with that
className?: string; | ||
} | ||
|
||
export type TypographyVariants = |
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 agree, I've split button styles into two separate props to reflect Figma. Your approach makes sense to me 👌
{small ? ( | ||
<Typography variant="body-m/semibold">{title}</Typography> | ||
) : ( | ||
<Typography variant="head-s/semibold">{title}</Typography> | ||
)} |
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.
maybe something like that:
{small ? ( | |
<Typography variant="body-m/semibold">{title}</Typography> | |
) : ( | |
<Typography variant="head-s/semibold">{title}</Typography> | |
)} | |
<Typography variant={small ? "body-m/semibold" : "head-s/semibold"}>{title}</Typography> |
No description provided.