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!: Split text tokens into ‘body text’ and ‘heading’ groups and rename to ‘typography’ #1845

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

Conversation

VincentSmedinga
Copy link
Contributor

@VincentSmedinga VincentSmedinga commented Feb 1, 2025

Describe the pull request

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

What

Refactors typography tokens:

  • Renames ams.text to ams.typography to match NLDS
  • Splits font size, line height and font weight tokens into body-text and heading groups to prepare for separate line height values

Why

To implement a stable API of brand tokens for typography.

How

(How were these changes implemented? Provide a brief overview of the approach taken and any key considerations.)

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.

Additional notes

  • Next up: update line height values in a separate PR
  • The bottom half of the Typography documentation is a bit out of place, but hasn’t changed in this PR. Out of scope.

# Conflicts:
#	proprietary/tokens/src/components/ams/accordion.tokens.json
#	proprietary/tokens/src/components/ams/avatar.tokens.json
#	proprietary/tokens/src/components/ams/badge.tokens.json
#	proprietary/tokens/src/components/ams/blockquote.tokens.json
#	proprietary/tokens/src/components/ams/character-count.tokens.json
#	proprietary/tokens/src/components/ams/checkbox.tokens.json
#	proprietary/tokens/src/components/ams/date-input.tokens.json
#	proprietary/tokens/src/components/ams/description-list.tokens.json
#	proprietary/tokens/src/components/ams/error-message.tokens.json
#	proprietary/tokens/src/components/ams/field-set.tokens.json
#	proprietary/tokens/src/components/ams/heading.tokens.json
#	proprietary/tokens/src/components/ams/label.tokens.json
#	proprietary/tokens/src/components/ams/ordered-list.tokens.json
#	proprietary/tokens/src/components/ams/page-heading.tokens.json
#	proprietary/tokens/src/components/ams/pagination.tokens.json
#	proprietary/tokens/src/components/ams/paragraph.tokens.json
#	proprietary/tokens/src/components/ams/password-input.tokens.json
#	proprietary/tokens/src/components/ams/radio.tokens.json
#	proprietary/tokens/src/components/ams/search-field.tokens.json
#	proprietary/tokens/src/components/ams/select.tokens.json
#	proprietary/tokens/src/components/ams/skip-link.tokens.json
#	proprietary/tokens/src/components/ams/switch.tokens.json
#	proprietary/tokens/src/components/ams/table.tokens.json
#	proprietary/tokens/src/components/ams/text-area.tokens.json
#	proprietary/tokens/src/components/ams/text-input.tokens.json
#	proprietary/tokens/src/components/ams/time-input.tokens.json
#	proprietary/tokens/src/components/ams/top-task-link.tokens.json
#	proprietary/tokens/src/components/ams/unordered-list.tokens.json
@github-actions github-actions bot temporarily deployed to demo-DES-1073-refactor-text-level-scale February 1, 2025 23:09 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-1073-refactor-text-level-scale February 4, 2025 12:57 Destroyed
# Conflicts:
#	proprietary/tokens/src/components/ams/accordion.tokens.json
#	proprietary/tokens/src/components/ams/blockquote.tokens.json
#	proprietary/tokens/src/components/ams/description-list.tokens.json
#	proprietary/tokens/src/components/ams/figure.tokens.json
#	proprietary/tokens/src/components/ams/heading.tokens.json
#	proprietary/tokens/src/components/ams/ordered-list.tokens.json
#	proprietary/tokens/src/components/ams/page-heading.tokens.json
#	proprietary/tokens/src/components/ams/paragraph.tokens.json
#	proprietary/tokens/src/components/ams/unordered-list.tokens.json
# Conflicts:
#	proprietary/tokens/src/components/ams/accordion.tokens.json
#	proprietary/tokens/src/components/ams/button.tokens.json
#	proprietary/tokens/src/components/ams/checkbox.tokens.json
#	proprietary/tokens/src/components/ams/file-input.tokens.json
#	proprietary/tokens/src/components/ams/radio.tokens.json
#	proprietary/tokens/src/components/ams/search-field.tokens.json
#	proprietary/tokens/src/components/ams/switch.tokens.json
#	proprietary/tokens/src/components/ams/tabs.tokens.json
@github-actions github-actions bot temporarily deployed to demo-DES-1073-refactor-text-level-scale February 12, 2025 13:49 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-1073-refactor-text-level-scale February 12, 2025 13:55 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-1073-refactor-text-level-scale February 12, 2025 13:57 Destroyed
@VincentSmedinga VincentSmedinga changed the title feat!: Change all text tokens into ‘typography’, ‘body text’ and ‘headings’ feat!: Split text tokens into ‘body text’ and ‘heading’ groups and rename to ‘typography’ Feb 12, 2025
@github-actions github-actions bot temporarily deployed to demo-DES-1073-refactor-text-level-scale February 12, 2025 16:06 Destroyed
proprietary/tokens/src/brand/ams/typography.tokens.json Outdated Show resolved Hide resolved
Comment on lines 8 to 9
"font-size": { "value": "{ams.typography.body-text.xl.font-size}" },
"line-height": { "value": "{ams.typography.body-text.xl.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.

Hmm, icons can be paired with both text and headings, and these have different line heights for the same text levels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alimpens We must update the Icon API for this. An icon can no longer just say “I’m with someone on text level 4”, because that could be either a heading (with a line height of 1.3) or a large paragraph (with 1.5).

How about

size: 'small' | 'medium' | 'large' | 'extra-large' | 'heading-0' … 'heading-6' ?

@VincentSmedinga VincentSmedinga marked this pull request as ready for review February 12, 2025 16:12
@VincentSmedinga VincentSmedinga requested a review from a team as a code owner February 12, 2025 16:12
@github-actions github-actions bot temporarily deployed to demo-DES-1073-refactor-text-level-scale February 12, 2025 16:17 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-1073-refactor-text-level-scale February 12, 2025 17:50 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-1073-refactor-text-level-scale February 14, 2025 12:34 Destroyed
proprietary/tokens/src/brand/ams/typography.tokens.json Outdated Show resolved Hide resolved
proprietary/tokens/src/brand/ams/typography.tokens.json Outdated Show resolved Hide resolved
"normal": { "value": 400 },
"bold": { "value": 800 }
"body-text": {
"font-size": { "value": "clamp(1.125rem, calc(1.0313rem + 0.4688vw), 1.5rem)" },
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 want to communicate that the 2 lists partially overlap, we could use a reference here. Something like {ams.typography.heading.5.font-size}. That would also prevent inconsistencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s because we made our ‘layer above brand’ invisible. You could say we lost the same kind of information in our colour tokens, where nothing explicitly indicates that our invalid red and error red stem from the same colour code.

So, relating a body text size to a heading size is incorrect – they have the same ancestor. Even more so because the only reason for a ‘level 5 heading’ font size to exist is to allow a Heading to match the font size of a paragraph.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, what I mean is more: do we want to communicate this invisible layer to our users? Relating a body text size to a heading size isn't strictly correct, but we do communicate possibly valuable information to our users if we do.

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