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

[NO-TICKET] Fix up text tokens and usage #2784

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

pwolfert
Copy link
Contributor

@pwolfert pwolfert commented Nov 4, 2023

Summary

Somewhat related to #2776

  • Removed misnamed typography-heading-base__font-size token, which really describes body text
  • Use the theme-level font-size variables in the body-text CSS instead of the heading variables
    • I could have made variables for these under typography-body, but they don't differ between themes...but then again, maybe we need these for Sketch?

@pwolfert pwolfert requested a review from zarahzachz November 4, 2023 00:45
@pwolfert pwolfert added this to the 9.0.0 milestone Nov 6, 2023
@pwolfert pwolfert added Type: Breaking This item causes a breaking change. Impacts: Core Impacts the core DS primarily, changes may occur in other themes as well. labels Nov 6, 2023
@pwolfert
Copy link
Contributor Author

pwolfert commented Nov 6, 2023

@zarahzachz, this is a draft because I would like your feedback about the approach here. See the description

@zarahzachz
Copy link
Collaborator

i actually prefer moving away from "heading" and "body" token names - at least from a theming perspective when it comes to font-size. i believe every system i've encountered does this sorta thing, from tailwind to open props to utopia.

it's like having a color palette to choose from, only you now have a font-size palette. more flexible, more maintainable, and i think more intuitive (you don't have to remember edge cases and rules to use one size with another). this approach might also lend itself to icon sizing since its more agnostic.

@pwolfert
Copy link
Contributor Author

pwolfert commented Nov 6, 2023

@zarahzachz, given the constraints of this problem where we want to be able to define the properties of heading components/elements/classes in the theme definitions, how would you structure and define this data? Some pieces of information that would need to live somewhere (CSS, theme files, and/or Sketch):

  • Core, Healthcare, and CMS.gov have a different font weight for "small" headings
  • Medicare uses the same weight for all headings
  • Medicare uses a different font family for headings and body text
  • Core, Healthcare, and CMS.gov use the same font family for headings and body text

@zarahzachz
Copy link
Collaborator

ok i have a clearer idea of what you're asking now.

i think using the higher up tokens (more abstract ones) to define font-sizing for body text makes sense rn because all themes share the same body font sizing. i could see the eventual need for more granular size tokens after the layout/type scale work is done, but in the meantime this solution works fine imo

@pwolfert pwolfert marked this pull request as ready for review November 13, 2023 21:51
@pwolfert pwolfert merged commit 0c2b156 into main Nov 14, 2023
@pwolfert pwolfert deleted the pwolfert/fix-misused-text-tokens branch November 14, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impacts: Core Impacts the core DS primarily, changes may occur in other themes as well. Type: Breaking This item causes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants