[Typography] Add semantic type tokens and block new raw font sizes - #96880
Conversation
|
🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
| <Text | ||
| fontSize={variables.fontSizeLabel} | ||
| variant="label" |
|
🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
# Conflicts: # config/eslint/eslint.seatbelt.tsv
# Conflicts: # config/eslint/eslint.seatbelt.tsv
# Conflicts: # config/eslint/eslint.seatbelt.tsv
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
grgia
changed the title
|
@aimane-chnaif yes ready for review |
# Conflicts: # config/eslint/eslint.config.mjs # config/eslint/eslint.seatbelt.tsv
|
I made the change in the issue thread |
|
🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@aimane-chnaif bump on review |
|
Reviewing |
aimane-chnaif
left a comment
aimane-chnaif
left a comment
There was a problem hiding this comment.
Some finds based on code review: (still testing & verifying)
| fontSize, | ||
| textAlign, | ||
| ...FontUtils.fontFamily.platform[family], | ||
| ...(variant ? textVariants[variant] : undefined), |
There was a problem hiding this comment.
variant overrides explicitly-passed fontSize and family
| fontSize: variables.fontSizeLabel, | ||
| lineHeight: variables.lineHeightLarge, | ||
| fontSize: fontScale.label, | ||
| lineHeight: lineHeightScale.label, |
There was a problem hiding this comment.
label line height drops 18 → 16
| microStrong: { | ||
| ...fontFamilyScale.strong, | ||
| fontSize: fontScale.micro, | ||
| lineHeight: lineHeightScale.micro, | ||
| }, |
There was a problem hiding this comment.
microStrong line box is shorter than its glyphs at high pixel ratio
| textMicroBold: { | ||
| ...textVariants.microStrong, | ||
| color: theme.text, | ||
| ...FontUtils.fontFamily.platform.EXP_NEUE_BOLD, | ||
| fontSize: variables.fontSizeSmall, | ||
| lineHeight: variables.lineHeightNormal, | ||
| }, | ||
|
|
||
| textMicroBoldSupporting: { | ||
| ...textVariants.microStrong, |
There was a problem hiding this comment.
textMicroBold line box becomes smaller than its own glyphs at large text sizes
| main | PR | |
|---|---|---|
| fontSize | 11 / 17 | 11 / 17 |
| lineHeight | 16 / 21 | 14 / 16 |
At max accessibility text size the font caps at 17px inside a 16px line box, so descenders clip. Affects 12 files.
Repro:
- iOS Settings → Accessibility → Display & Text Size → Larger Text → enable and drag the slider to maximum
- Open New Expensify → Search tab
- Tap any filter pill at the top (Type / Status / Date)
- Pill labels (
src/components/Search/FilterDropdowns/DropdownButton.tsx:97) render clipped vs. main
src/components/Table/TableHeader.tsx:279is also affected aslh16sits first in the array:
App/src/components/Table/TableHeader.tsx
Line 279 in d57ad07
There was a problem hiding this comment.
Above case is not reproducible but there might be other places where text bottom is cut off.
Not found so far.
| /> | ||
| </View> | ||
| <Text style={[styles.textHeadline, styles.textXXLarge, styles.textAlignCenter]}>{translate('validateCodeModal.expiredCodeTitle')}</Text> | ||
| <Text style={[styles.textHeadline, styles.textAlignCenter]}>{translate('validateCodeModal.expiredCodeTitle')}</Text> |
There was a problem hiding this comment.
Six full-screen headings shrink ~21% at normal text size
styles.textXXLarge was dropped from six headings, so they fall back to textHeadline's size:
- fontSize 28 → 22 at normal device text size (both are 28 at max, so this only shows at normal settings)
- lineHeight 32 → 28
Screens: SAMLLoadingIndicator.tsx:30, ValidateCode/ExpiredValidateCodeModal.tsx:40, ValidateCode/JustSignedInModal.tsx:50, ValidateCode/ValidateCodeModal.tsx:65, pages/ConnectionCompletePage.tsx:29, pages/ErrorPage/SessionExpiredPage.tsx:36
Repro:
- Sign out
- Enter your email → request a magic code
- Open the emailed link twice (the second open hits the expired path)
- The "Security code has expired" headline is visibly smaller than on main
Direct route without the email round-trip: /connection-complete
There was a problem hiding this comment.
Here's comparison
this branch:
production:
Intentional per commit history, but user-visibleThese trace to explicit commits ("Align headline, label, and micro strong values with the Figma library", "Size suggestion menu emojis like inline emojis and labels like body text"). Flagging so the blast radius is a conscious call:
Repro for the label change:
|
|
I performed full regression test as many cases as possible but I might have missed places where bottom part of text is cut off due to reduced |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
aimane-chnaif
left a comment
aimane-chnaif
left a comment
There was a problem hiding this comment.
Please check NAB comments above
| mutedTextLabel: { | ||
| color: theme.textSupporting, | ||
| fontSize: variables.fontSizeLabel, | ||
| lineHeight: variables.lineHeightLarge, | ||
| fontSize: fontScale.label, | ||
| lineHeight: lineHeightScale.label, | ||
| }, | ||
|
|
||
| mutedNormalTextLabel: { | ||
| color: theme.textSupporting, | ||
| fontSize: variables.fontSizeLabel, | ||
| lineHeight: variables.lineHeightNormal, | ||
| fontSize: fontScale.label, | ||
| lineHeight: lineHeightScale.label, |
There was a problem hiding this comment.
Now 2 style keys have exact same attributes (same color, same size, same line height)
| textHeadlineH1: { | ||
| ...headlineFont, | ||
| ...whiteSpace.preWrap, | ||
| ...textVariants.h1, | ||
| color: theme.heading, | ||
| fontSize: variables.fontSizeXLarge, | ||
| lineHeight: variables.lineHeightSizeH1, | ||
| }, |
There was a problem hiding this comment.
Now textHeadlineH1 is exactly the same as textHeadline
| const shouldShowCheckbox = isSelectionModeEnabled || !shouldUseNarrowLayout; | ||
|
|
||
| const textStyle = shouldUseNarrowLayout ? {fontSize: variables.fontSizeLabel, lineHeight: 16} : [styles.labelStrong]; | ||
| const textStyle = shouldUseNarrowLayout ? {fontSize: variables.fontSizeLabel, lineHeight: variables.lineHeightNormal} : [styles.labelStrong]; |
There was a problem hiding this comment.
This inline style is now exactly the same as styles.label
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61dbba653e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| context.report({ | ||
| node: valueNode, | ||
| messageId: 'rawTypography', | ||
| data: { |
There was a problem hiding this comment.
When a style is built or mutated with an assignment such as style.fontSize = 17 or style.lineHeight = 20, the parser emits an AssignmentExpression, so neither listener runs and the new lint gate accepts the raw typography value. This is valid for a TextStyle in both TypeScript and JavaScript; add handling for statically named member assignments and cover both properties in the rule tests.
Useful? React with 👍 / 👎.
ReviewCode LGTM. The token layer, the What I checked in the code
One minor thing worth a look: in
|
| Step | Status | Observation |
|---|---|---|
| App boots past splash | ❌ fail | snapshot returned 0 nodes across 6+ min; screenshots minutes apart are pixel-identical; all bundles/fonts 200 |
| 1 — chat text rendering | ❌ blocked | app never reached any screen |
| 2 — Share code QR heading/subtitle | ❌ blocked | app never reached any screen |
| 3 — workspace section titles/labels | ❌ blocked | app never reached any screen |
| 4 — empty-chat welcome headline | ❌ blocked | app never reached any screen |
| 5 — magic-code modal heading size | ❌ blocked | app never reached any screen |
Screenshots of the boot-splash hang:
Evidence (2)
Local test app stuck on boot splash during prestart sign-in
Identical boot splash ~6 min later — app never mounted
Regression scanNo logic/functional regressions found. The risk in this PR is purely value drift — a 1. Autocomplete suggestion menus resized (not in the description) — worth confirming with design
This is a deliberate commit ("Size suggestion menu emojis like inline emojis and labels like body text"), but it's not listed in the "aligns the drifted values" section, so a reviewer/QA reading the description wouldn't know to check the 2.
|
|
🚧 blimpich has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.62-1 🚀
|
Help site review: no changes requiredI reviewed the changes in this PR against the help site content in Why: This PR is an internal frontend/design-system refactor. It:
None of this changes a customer-facing feature, workflow, setting, tab, or button. The help site articles document how to use Expensify (billing, workspaces, expenses, integrations, etc.) — they never document typography tokens, font sizes, or line heights. The only user-visible effects here are subtle rendering refinements (slightly tighter line heights, a smaller magic-code modal heading), which aren't described in any article. Since no help site changes are required, I did not create a draft docs PR. @grgia, if you believe a specific article is affected by a behavior change I missed, let me know which one and I'll take another look. |
6 participants
Explanation of Change
Text styles now come from a typography token layer (
src/styles/typography.ts), and new rawfontSize/lineHeightliterals are blocked by lint.Type styles have drifted with nothing to stop it:
fontSizeH1andfontSizeH2are both 19, and ~75text*styles express about a dozen real roles because color is baked into type styles. The token file defines a primitive scale plus semantic variants named after the Figma type styles, and the existingtext*styles are aliased to those tokens with color re-attached at the alias boundary. Design confirmed the Figma library is the source of truth for values, so this PR also aligns the drifted values (headline line height 32→28, label 18→16, micro strong 16→14, intro headline 45→44, extra small button text line height to 12) and moves the oversized Kansas 28 headings on the loading, magic link, and confirmation screens ontoH1(22/28), per the design decisions on the issue. Everything else renders exactly as before.Product/*styles 1:1H1<Text variant="...">added,fontSizeprop deprecatedeslint-seatbeltQRSharemigrated as the worked exampleintroHeadlinematches the new FigmaProduct/Intro HeadlinestyleFixed Issues
$ #37503
PROPOSAL:
Tests
npm run test -- tests/unit/NoRawTypographyRuleTest.ts tests/unit/TextVariantTest.tsxand verify both suites pass.fontSize: 17to any component style, runnpm run linton that file, and verify therulesdir/no-raw-typographyerror appears.main.Offline tests
None, style definitions only. No network-dependent behavior.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari