[TV] Align Android TV UI with the Figma design-system tokens - #5701
[TV] Align Android TV UI with the Figma design-system tokens#5701sztomek wants to merge 15 commits into
Conversation
TV design-system alignment — discrepancies to reviewThe Android TV UI was re-pointed onto the Figma design-system tokens (new
Policy applied
Colors — value / alpha changes to verify
Removed legacy token with no Figma equivalent (was unused): Resting translucent-white note: buttons use a 20%-white resting background paired with a New tokens defined but not yet consumed anywhere: Text styles — re-pointed (non-exact matches)
Text styles — formerly "kept as-is", now re-pointed (Figma style names confirmed)All per-component styles have been eliminated;
The welcome styles' custom negative letter-spacing was dropped (not on the ramp); centering is Also left as Material typography (no ramp match): empty-state / error subtitles use Misc
|
|
Claude finished @sztomek's task in 5m 29s —— View job Code reviewNice, disciplined re-point — the commit split (tokens → tiles → rows → modals → nav → onboarding → cleanup) makes it reviewable, and the discrepancy table is genuinely useful. I verified no dangling references to the removed tokens remain, so the module still compiles. I found one blocking issue and a handful of things worth deciding before merge. Detailed notes are in inline comments; summary below. 🔴 Blocking —
|
| File | Line |
|---|---|
component/TvEmptyState.kt |
18 |
upnext/TvUpNextScreen.kt |
25 |
onboarding/createaccount/TvCreateAccountModal.kt |
17 |
onboarding/welcome/TvWelcomeScreen.kt |
23 |
onboarding/signin/TvSignInScreen.kt |
22 |
onboarding/signin/TvSyncingScreen.kt |
31 |
playlists/TvPlaylistsScreen.kt |
39 |
playlists/details/TvPlaylistDetailsScreen.kt |
29 |
podcasts/TvFolderDetailScreen.kt |
20 |
podcasts/TvPodcastDetailsScreen.kt |
34 |
podcasts/TvYourPodcastsScreen.kt |
20 |
./gradlew spotlessApply clears all 11. The PR checklist has the linter box ticked, so it's worth re-running — I wasn't able to execute Gradle in this environment (no permission for ./gradlew), so this is from static analysis: each of these files has zero remaining Color-prefixed references and no : Color type usage.
🟡 Worth fixing in this PR
1. Contradictory textAlign in TvCreateAccountModal.kt:68-69 — Headline.copy(textAlign = TextAlign.Center) is immediately overridden by the textAlign = TextAlign.Start parameter. The Text param wins, so behaviour is unchanged, but it's dead code that reads as a bug. This header is deliberately left-aligned; the Center shouldn't have been carried over from ModalTitle. (inline)
2. TextTertiaryActive == TextTertiary (TvColors.kt:14) — both #7A7D82. Every other *Active pair inverts for light surfaces; a mid-grey that's unchanged on both #161718 and #FBFBFC reads like a copy/paste slip. Also only ~3.9:1 on bg-active, so it'd fail AA if wired up. Please re-check against Figma. (inline)
3. Ramp tokens no longer pin letterSpacing — the removed TabLabel set letterSpacing = 0.sp; Caption1 doesn't. tv.material3.Text does LocalTextStyle.current.merge(style), and merge only takes the argument's value when specified, so ambient Material tracking now leaks into the tab labels. Small visual delta, but it also makes all eight ramp tokens ambient-dependent, which undercuts the point of having canonical tokens. (inline)
4. Title1, Title2, Subtitle1 have no call sites — the description accounts for the unused colours explicitly, but not these; Subtitle1 doesn't appear in any table. If they're deliberate placeholders, a one-line KDoc on TvTextStyles saying so (plus documenting the Figma-px ÷ 1.5 convention and the SF Pro omission) would prevent both "dead code" review comments and someone later "tidying" the fractional sp values.
🟢 Nits / follow-ups
- Token semantics for non-text foregrounds.
LoadingView(color = TvColors.TextPrimary)appears in 9 places, andiconButtonColorsusesTextPrimary/TextPrimaryActivefor icon tints. Values are right, names aren't — aForegroundPrimaryalias (or the Figma foreground token, if one exists) would keep the mapping honest. (inline) .copy(textAlign = Center)per recomposition inTvFolderCard.kt:85,TvEpisodeActionsModal.kt:118,TvProfileModal.kt:86/:178,TvDownloadAppModal.kt:59.TextStylehas a properequals, soTextstill skips — it's just a needless allocation per cell per recomposition. SinceCaption1.copy(textAlign = Center)is repeated in three files, a sharedTvTextStyles.Caption1Centeredwould be tidier than five local.copycalls. (inline)TvFolderCardtruncation.Caption2is 15.33sp vs the old 14sp — ~10% wider glyphs on amaxLines = 1, softWrap = falselabel in a fixed-width card. Worth checking a long folder name; the preview uses a short one.TvTopBar.kt:75still uses literalColor.Whitefor the focus-ring border while the container it wraps is nowBackgroundActive(#FBFBFC). Imperceptible, but since the border exists specifically to reinforce the focused container,TvColors.BackgroundActivewould be more consistent than leaving it as a "functional white".- Focused + selected tile buttons lose their white pill (
focused*colours ignoreisSelected), so focusing a selected item reads as deselecting it, andTextSecondaryon 20% white is ~4:1. Pre-existing, but adjacent to this PR's stated focus-legibility goal. (inline) Title3is +4sp on every screen title (ScreenTitle28 → 32). It lands in aRowwithweight(1f)next toTvArchivedFilterButtoninTvPodcastDetailsScreen.kt:295, and above fixed paddings inTvPodcastGridScaffold/TvPlaylistsScreen/TvUpNextScreen. Probably fine, but this is the change most likely to shift layout — the "screenshots to be attached" would settle it.
Things I checked and found clean
- No dangling references to any removed token (
Dark,DarkGray,Gray,LightGray,TextPrimary20,BgActive,BgActive20,Divider, or the 11 removed text styles) — the module compiles. - Alpha encodings are exact:
TextPrimary700xB3= 0.702 ✓ matches the old.copy(alpha = 0.7f);BackgroundActive500x80= 0.502 ✓;BackgroundActive200x33= 0.2 ✓ (so the resting-button backgrounds really are a no-op, as the description claims). - Every previously-centered style that got re-pointed has centering restored where the label is full-width, and correctly omitted where it isn't (
TvDownloadAppModal.kt:87— the Done button isn'tfillMaxWidth, soCaption1withoutCenteris right). - Focused-state contrast on the changed surfaces holds up:
TextPrimaryActive#161718andTextSecondaryActive#3D4044onBackgroundActive#FBFBFCare ~18:1 and ~10:1. - No security or data-handling surface in this diff — it's colours and text styles only. No analytics, strings, or DB changes, consistent with the checklist.
Note the two unchecked UI-testing boxes (large display / font size, TalkBack). Large-font is the relevant one here: the ramp uses sp throughout, so a +4sp base on screen titles compounds under font scaling.
· branch feat/tv-design-system-tokens
Generated by 🚫 Danger |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
sztomek
changed the title
446584f to
3809384
Compare
|
Claude finished @sztomek's task in 5m 9s —— View job Code reviewRe-reviewed after the
✅ Fixed since last round
🟡 Worth resolving before merge1. Featured tile hierarchy is inverted. The 2. Show notes / podcast description are on the smallest ramp step in secondary grey. 3. Onboarding titles are +88%, unverified in a long locale. 4. 5. 6. The PR description tables no longer match the code. Since they're what design will review against, they're worth refreshing:
🟢 Nits / carried over
Things I checked and found clean
Caveat: I couldn't run Gradle in this environment (no permission for |
geekygecko
left a comment
geekygecko
left a comment
There was a problem hiding this comment.
Thanks for making this change!
| * convention), hence the fractional `sp`. Figma's SF Pro family is not ported (no such resource in | ||
| * this module), so the tokens use the system default at [FontWeight] 510. The full ramp is defined | ||
| * even where a step has no call site yet, so the palette stays complete. | ||
| */ |
There was a problem hiding this comment.
I'm not sure if it helps but there is a page in Figma related to the text styles for Google.
| Text( | ||
| text = pluralStringResource(LR.plurals.episode_count, episodeCount, episodeCount), | ||
| style = TvTextStyles.PlaylistCardCaption, | ||
| style = TvTextStyles.Caption2, |
There was a problem hiding this comment.
These don't match the text style from the Figma document, but maybe we can do this separately later, as this PR is already quite large. This change would also make the text bigger, and it really seems quite large for this card.
…int remaining tile text colors
3809384 to
fc3bc34
Compare
|
Claude finished @sztomek's task in 4m 37s —— View job Code reviewRe-reviewed after
✅ Fixed since last round
📐 Figma cross-check (from @geekygecko's screenshot)The Google TV styles → Text Styles page is a useful confirmation, and worth folding back into the code:
🟡 One thing I'd still resolve before mergeShow notes / podcast description are on the smallest ramp step in secondary grey. 🟢 Carried over — fine as follow-ups
Correcting my round-1 reviewI flagged unused Things I checked and found clean
Caveat: |
| * The Figma type ramp for TV. Sizes are the Figma px values divided by 1.5 (the 1920x1080 → dp | ||
| * convention), hence the fractional `sp`. Figma's SF Pro family is not ported (no such resource in | ||
| * this module), so the tokens use the system default at [FontWeight] 510. The full ramp is defined | ||
| * even where a step has no call site yet, so the palette stays complete. |
There was a problem hiding this comment.
The Figma page @geekygecko linked (Google TV styles → Text Styles) settles two of this KDoc's open questions, and contradicts one of them:
- The ÷1.5 convention is confirmed. The selected
Caption 1in that screenshot reads25 / 32 / 0%, and this file'sCaption1is16.67sp / 21.33sp / 0.sp— exactly25÷1.5and32÷1.5. Nice to have that verified rather than assumed. - The family is Google Sans, not SF Pro. The Typography panel shows
Google Sans · Medium · 25 / 32. The PR description's "Figma specifiesFontFamily(R.font.sf_pro)(iOS)" appears to have come from a different (iOS) page; the TV page is Google Sans. The conclusion is unchanged — neither font ships in this repo (modules/services/ui/src/main/res/font/has onlyroboto_serifanddm_sans), so the system default is still the right fallback — but the KDoc currently sends the next reader looking for the wrong font. FontWeight(510)came from the SF Pro reading. Google Sans Medium isFontWeight(500). Both round to the same Roboto face on Android TV so there's no visual delta, but 500 is the value the design actually specifies and it drops the "why 510?" question.
| * The Figma type ramp for TV. Sizes are the Figma px values divided by 1.5 (the 1920x1080 → dp | |
| * convention), hence the fractional `sp`. Figma's SF Pro family is not ported (no such resource in | |
| * this module), so the tokens use the system default at [FontWeight] 510. The full ramp is defined | |
| * even where a step has no call site yet, so the palette stays complete. | |
| * The Figma type ramp for TV (Google TV styles → Text Styles). Sizes are the Figma px values | |
| * divided by 1.5 (the 1920x1080 → dp convention), hence the fractional `sp`. Figma's Google Sans | |
| * family is not ported (no such resource in this repo), so the tokens use the system default at | |
| * [FontWeight] 500, matching Google Sans Medium. The full ramp is defined even where a step has no | |
| * call site yet, so the palette stays complete. |
(If you take the KDoc change, the FontWeight(510) on the nine tokens below wants to become FontWeight(500) to match.)
| text = remember(showNotes.html) { AnnotatedString.fromHtml(showNotes.html) }, | ||
| style = TvTextStyles.FeaturedTileDescription, | ||
| color = Color.White, | ||
| style = TvTextStyles.Caption2, |
There was a problem hiding this comment.
Carried over from last round and still the one substantive thing I'd change before merge — the other four points from that review are now fixed, so flagging it once more rather than letting it slip through on the approval.
Show notes are the longest-form reading surface in the TV app, and they're on the smallest ramp step in secondary grey: Caption2 15.33sp / #B0B3B8, down from 16sp / Color.White. Same on the podcast description at TvPodcastInfoModal.kt:186. Meanwhile Body (19.33sp) — the token literally named for body copy — is used for one-line subtitles and error strings (TvCreateAccountScreen.kt:73, TvSignInScreen.kt:127, TvDownloadAppModal.kt:64, TvToast.kt:101) but for neither description pane. At 10-foot viewing distance the scroll-heavy panes are where the extra 4sp matters most.
The PR description also still lists ModalBody → Body, which is what makes this read as drift from the "Fine tune text styles" pass rather than a deliberate call.
| style = TvTextStyles.Caption2, | |
| style = TvTextStyles.Body, | |
| color = TvColors.TextPrimary, |
| Text( | ||
| text = pluralStringResource(LR.plurals.episode_count, episodeCount, episodeCount), | ||
| style = TvTextStyles.PlaylistCardCaption, | ||
| style = TvTextStyles.Caption2, |
There was a problem hiding this comment.
Adding the concrete numbers to @geekygecko's point, since they make the "defer it" call easy to justify in writing rather than from memory:
Figma has the episode count at Caption 1 · 25/32, i.e. 16.67sp / 21.33sp after the ÷1.5 — this is Caption2 (15.33sp / 20sp). So the gap is 1.34sp, about 9%. The Smart Playlist caption on line 117 is the same token and presumably the same Figma style.
Worth noting for whoever picks up the follow-up: this card puts both captions in a Column with a weight(1f) spacer between them, and the title above is maxLines = 2 Headline (25.33sp, 30.67sp line height). In the fixed-height card that's ~61dp of title plus two caption lines, so a 1.34sp bump on each caption is ~3dp of extra vertical demand — tight but not obviously overflowing. Cheap to verify with a two-line playlist name.
Agreed it doesn't need to block this PR.
3 participants
Description
Aligns the Android TV UI with the Figma design system by extracting reusable color and text-style tokens and re-pointing every TV screen/component to them.
TvColorsnow defines the full Figma palette:TextPrimary/Secondary/Tertiary/Disabled(+ their*Activevariants)TextPrimary70, and backgroundsBackgroundSunken/Surface/Base/Overlay/Active(+BackgroundActive50/20).TvTextStylesgains the Figma type rampTitle1…Caption2(+Body) (Figma px ÷ 1.5 per the TV module convention; system font kept,FontWeight(510)).Dark,DarkGray,Gray,LightGray,TextPrimary20,BgActive,BgActive20,Divider) and the now-unused per-component text styles removed.Where existing styling didn't map cleanly, the closest token was used and the gap logged (see below). Stacked on
feat/tv-deep-destination-top-bar(base of this PR).Decisions / discrepancies for design review
Gray#3C3E42 →BackgroundOverlay#323538; focus/active backgrounds#D4D6DB→BackgroundActive#FBFBFC (bg-active).BackgroundActive20) — Figma only definesbg-active-50; 50% would make the white label low-contrast. Needs a design call.Caption1,Body,Title1,Title2,Headline); aBodytoken (19.33sp / 400) was added to the ramp.TvTextStylesnow contains only ramp tokens.A full discrepancy list is in the PR comments.
Testing Instructions
Screenshots or Screencast
Screen_recording_20260805_181413.mp4
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml(no string changes)I have tested any UI changes...