-
Notifications
You must be signed in to change notification settings - Fork 0
Add typography component with story #9
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for contributing! |
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.
Looks good but I think we should be able to set the accessible heading level independently of 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 like that the font sizes are handled with classes and media queries. Could font-size and line-height be changed to be in terms of rem
? That way, if someone 🧓 has an increased font size for accessibility reasons, the text will be scaled up for them.
type Variant = 'displayName' | 'display1' | 'display2' | 'title1' | 'title2' | 'title3' | | ||
'bodyLarge' | 'bodyMedium' | 'bodySmall' | 'cardTitle' | 'cardBody' | 'cardTags'; | ||
|
||
type ComponentType = 'p' | 'span' | 'div'; |
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.
Should probably add h1
to h6
here
// As determined by: https://www.figma.com/design/9iQV86DfnonmL01vyEFqxq/Design-System-v2?node-id=19-2&t=3DSRR1vIg0O9KvWe-1 | ||
// @param props | ||
// @param props.variant - variant as specified by Diamond Design System, e.g. display name, card title, etc. | ||
// @param props.component - the actual semantic element, e.g. `h1`, `p`, `div` — overriden by props.variant if h1-6 |
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 the variant should dictate the semantic element to use; it should be the other way around. For example, if they skip heading levels in the design, we need to make sure that heading levels aren't skipped in the HTML. Or if they use (H4) Title 1 for the title of the page, we should be able to use <h1>
for that.
|
h1, h2, h3, h4, h5, h6, .bodyLarge, .bodyMedium, .bodySmall, .cardTitle, .cardBody, .cardTags { | ||
font-family: 'DM Sans', sans-serif; | ||
} |
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.
Think we need to import DM Sans or something here for it to apply, maybe check how main website does it
No description provided.