Skip to content
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

feat: Introduce and use brand and common typography tokens #1760

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

VincentSmedinga
Copy link
Contributor

@VincentSmedinga VincentSmedinga commented Nov 21, 2024

Describe the pull request

Thank you for contributing to the project!
Please use this template to help us handle your PR smoothly.

What

  1. Separates the ‘Text Level’ concept into headings and body text
  2. Moves the concept from the ‘brand’ level to ‘common’
  3. Prefixes both with ams.brand and ams.common
  4. Renames text to typography
  5. Introduces a new brand scale of line heights
  6. Removes the now obsolete ams.text tokens
  7. Configures Style Dictionary to log invalid tokens by name
  8. No changes in values other than the line heights

Why

  1. Headings and body text of levels 4, 5, and 6 share font sizes, but they don’t share line heights. For example, <Heading level={4}> needs a small line height, while <Paragraph size="large"> needs a large one. Rather than only splitting some levels into two (e.g. text-level-4-(heading|body-text)-line-height), we’ve separated the two concepts entirely.
  2. This also helped us to define our brand tokens more purely as real primitives.
  3. To visually separate the three levels, make it very obvious that common tokens use brand tokens and component tokens use common tokens, prevent naming problems, and keep names simple.
  4. To match NL Design System.
  5. To improve readability, the line heights have been made slightly smaller for headings and larger for body text. The scale is logarithmic, like font size, with a factor of 31/32 (0.96875). Not all large values are available.
  6. We no longer use them and don’t deprecate things yet on major version zero.
  7. To help find errors while refactoring.
  8. To keep this PR focused

How

  • Created new typography tokens for the ‘brand’ level
  • Created new typography tokens for the ‘common’ level
  • Applied them to all components using font-size, line-height, font-family and font-weight
  • Fine-tuned the hierarchy and naming of the tokens

Checklist

Before submitting your pull request, please ensure you have done the following. Check each checkmark if you have done so or if it wasn't necessary:

  • Add or update unit tests
  • Add or update documentation
  • Add or update stories
  • Add or update exports in index.* files
  • Start the PR title with a Conventional Commit prefix, as explained here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much more focused now: just our selection of font families, sizes and weights, and line heights, with proper names for their options.

"brand": {
"typography": {
"font-family": {
"body-text": { "value": "'Amsterdam Sans', Arial, sans-serif" },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including -text is clearer than just body. I dismissed body-copy for not being very common. Utrecht uses ‘document’, but headings are also in the document.

A typesetter […] sees 'body text' as being those sections of the main text that are flowed into columns or justified as paragraphs, as distinct from the headings and any pictures that are floated out of the main body.
Wikipedia

"typography": {
"font-family": {
"body-text": { "value": "'Amsterdam Sans', Arial, sans-serif" },
"headings": { "value": "'Amsterdam Sans', Arial, sans-serif" },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Headings include labels and fieldset legends as well. And accordion buttons use these tokens.

"font-family": {
"body-text": { "value": "'Amsterdam Sans', Arial, sans-serif" },
"headings": { "value": "'Amsterdam Sans', Arial, sans-serif" },
"monospace": { "value": "monospace" }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be too soon to add this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add a <code> component it might come in handy.

"bold": { "value": 700 },
"x-bold": { "value": 800 }
},
"line-height": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve calculated this scale precisely like we did for font sizes. I started with 1.66667 for paragraphs and found a factor of 31/32 to generated line heights very close to what we aimed for.

The 7 smallest are all used for consecutive heading levels. The 4 largest, creating 1 overlapping, for body text levels. Not all options (e.g. 4xl) have been made available.

"font-size": { "value": "{ams.brand.typography.font-size.lg}" },
"line-height": { "value": "{ams.brand.typography.line-height.2xl}" }
},
"xl": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new level for blockquotes because they are not headings.

"typography": {
"body-text": {
"font-family": { "value": "{ams.brand.typography.font-family.body-text}" },
"font-size": { "value": "{ams.brand.typography.font-size.md}" },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and line height apply to md – I chose not to make that level explicit, but to make these values the baseline, overridden by the variants, just like we do in CSS.

"font-weight": { "value": "{ams.brand.typography.font-weight.bold}" }
},
"headings": {
"font-family": { "value": "{ams.brand.typography.font-family.headings}" },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having two separate tokens for the font-family of headings and body text, while we use the same font for both, allows the community to use two different typefaces.

"line-height": { "value": "{ams.brand.typography.line-height.md}" }
}
},
"bold": {
Copy link
Contributor Author

@VincentSmedinga VincentSmedinga Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely sure about this one. Is a phrase or paragraph of bold text a variation? Badge uses it, for example. And blockquote. Does it make sense to let our future .ams-strong use this token?

We could also name it ‘emphasis’ and combine it with italics – normal and strong emphasis.

@VincentSmedinga VincentSmedinga marked this pull request as ready for review November 23, 2024 09:02
@@ -0,0 +1,17 @@
{
"ams": {
Copy link
Contributor

@dlnr dlnr Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To maintain the history of the file you can use git mv, just renaming it deletes it and adds a new file.

"font-family": {
"body-text": { "value": "'Amsterdam Sans', Arial, sans-serif" },
"headings": { "value": "'Amsterdam Sans', Arial, sans-serif" },
"monospace": { "value": "monospace" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add a <code> component it might come in handy.

"brand": {
"typography": {
"font-size": {
"sm": { "value": "clamp(0.8889rem, calc(0.9141rem + -0.0253vw), 0.9091rem)" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This grows smaller on desktop.

"light": { "value": 300 },
"normal": { "value": 400 },
"bold": { "value": 700 },
"x-bold": { "value": 800 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"bolder" might be more common.

"common": {
"typography": {
"body-text": {
"font-family": { "value": "{ams.brand.typography.font-family.body-text}" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to look at the bundler for the tokens here, because everything is bundled. So you're creating:

--ams-brand-typography-font-family and --ams-common-typography-body-text-font-family

Do we need both and how is it more semantic?

@@ -40,7 +40,7 @@
.sbdocs-content.sbdocs-content > .ams-storybook-status-badge > span:nth-child(2) {
color: #000;
font-family: "Amsterdam Sans", "Arial", sans-serif;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the custom property?

@@ -0,0 +1,61 @@
{
"ams": {
"common": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this prefix? We don't previx component properties ams-components-?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants