Skip to content

chore(design-tokens): 🤖 convert font sizing to rem for acessibility#966

Draft
punkbit wants to merge 84 commits intochore/design-tokens-packagefrom
chore/design-tokens-convert-size-to-rem-for-acessibility
Draft

chore(design-tokens): 🤖 convert font sizing to rem for acessibility#966
punkbit wants to merge 84 commits intochore/design-tokens-packagefrom
chore/design-tokens-convert-size-to-rem-for-acessibility

Conversation

@punkbit
Copy link
Copy Markdown
Collaborator

@punkbit punkbit commented Apr 2, 2026

Why?

Convert font size to "rem" (unit) for accessibility.

⚠️ TODO: await for #951 to include changeset

How?

  • Create root font size
  • Convert font pixel size to rem unit base in default browser's root 16px

Tickets?

N/A

Contribution checklist?

  • You've done enough research before writing
  • You have reviewed the PR
  • The commit messages are detailed
  • The build command runs locally
  • Assets or static content are linked and stored in the project
  • For documentation, guides or references, you've tested the commands

Security checklist?

  • All user inputs are validated and sanitized
  • No usage of dangerouslySetInnerHTML
  • Sensitive data has been identified and is being protected properly
  • Build output contains no secrets or API keys

Preview?

N/A

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 2, 2026

⚠️ No Changeset found

Latest commit: ae74740

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

Claude finished @punkbit's task in 2m 2s —— View job


PR Review

Thanks for this PR, @punkbit! The hybrid rem/px strategy is well-reasoned and the documentation in SPECIFICATION.md is thorough. The conversion math is correct throughout. 🙌


🚨 Must Fix

  • Missing changeset: Per the repo's CONVENTIONS.md, yarn changeset:add is required before merging if the public API changes. Converting font.size.* tokens from px to rem is a breaking change — consumers using the raw CSS variables (e.g., --cui-font-size-md) will now get 0.875rem instead of 14px. A changeset with a migration note is needed. Example: "Font size tokens now use rem units. If you're using the raw CSS variables in pixel-specific contexts, update accordingly. Assumes 1rem = 16px browser default." Fix this →

💡 Suggestions (non-blocking)

  • font.root footgun risk: The font.root primitive will be emitted as --cui-font-root: 16px in tokens-primitives.css (via the "font.**" pattern in terrazzo.config.ts). The SPECIFICATION.md guidance says consumers should apply html { font-size: var(--cui-font-root, 16px) }. However, if a consumer app already sets a custom html { font-size } without using this variable, rem values will silently resolve to unexpected sizes. Consider adding a warning comment directly in tokens-primitives.css (via a build step or header) or surfacing this more prominently in the package README.

  • .bak deletion mixed into this PR: The deletion of packages/figma-design-tokens-plugin.bak/ is good cleanup but unrelated to the rem migration. It significantly inflates the diff (−2,573 lines). Consider splitting into a separate cleanup PR to make this one easier to review and revert if needed.

  • Typo in branch/PR title: "acessibility" → "accessibility" (missing one c) — low-stakes but worth noting for the changelog.


Overall the change is correct and the rationale is solid. The main blocker is the missing changeset.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

Claude finished @punkbit's task in 1m 25s —— View job


PR Review

Good accessibility-focused change! The hybrid approach (rem for fonts, px for layout) is well-reasoned and documented. The cleanup of figma-design-tokens-plugin.bak is a nice bonus. Here's what I found:


🚨 Must Fix

  • Missing changeset: Changing CSS variable values from px to rem is a breaking change for any consumer using --cui-font-size-* variables. Per CONVENTIONS.md, a changeset is required for public API changes. Run yarn changeset:add and add a migration note (e.g., "Font size tokens now output rem instead of px. Ensure html { font-size: 16px } or html { font-size: var(--cui-font-root, 16px) } is set in consumer apps."). Fix this →

💡 Suggestions (non-blocking)

  • font.root consumer guidance: The font.root token (--cui-font-root: 16px) is well-documented in the SPEC, but consumers may not read it. Consider adding a note to the package README.md explaining that consumer apps should set html { font-size: var(--cui-font-root, 16px); } to ensure rem values resolve correctly. Without this, if a consumer has a different root font-size, all rem values will be off.

  • 0.625rem (10px) at non-default font sizes: The font.size.xs token resolves to 10px at default 16px root. If a user increases their browser font size to 20px, this scales to 12.5px — still acceptable. Just worth noting this is already near the WCAG minimum for non-body text.

  • SPEC rename cleanup: The SPEC previously referenced numeric naming (font/size/50, font/size/75...) but the actual tokens already used t-shirt sizes (xs, sm...). The spec update correctly aligns the docs with reality — good catch.


Overall: solid, well-documented approach. The main ask is the changeset — everything else looks good. 🙌

@workflow-authentication-public
Copy link
Copy Markdown
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-q7ignzt4i-clickhouse.vercel.app

Built from commit: 94f644803806dc53ef22bac5f2464a2803fc191a

@punkbit punkbit force-pushed the chore/design-tokens-package branch from 814d7ad to 6385b7a Compare April 2, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant