[TV] Provide design tokens through a TvTheme provider - #5703
Conversation
Claude is reviewing this PR…
|
Generated by 🚫 Danger |
sztomek
added
[Type] Enhancement
|
Claude finished @sztomek's task in 7m 18s —— View job Code review
Solid refactor — the structure mirrors the phone app's idiom closely and the mechanical part is clean. I verified the things most likely to go wrong in a 985/985 rename-heavy PR and they check out; the substantive comments are all about the derived tv-material3 scheme, which is the genuinely new behavior here. What I verified
Findings1. The tv-material3's 2. The retry button isn't the only default-styled component ( Both featured-tile 3. Duplicate values make
4. The one place the migration produced a new idiom instead of a substitution: PascalCase names on 5. Locals: silent fallback + dual access path (inline · Nothing needs the Optional
Not coveredI couldn't run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude finished @sztomek's task in 4m 55s —— View job Code review — incremental (new push)
One new commit since my last pass: That commit is the right fix, and it fully resolves finding 1I re-derived it end to end rather than taking the message at face value:
Also checked One description update needed: the "Deliberate visual changes" section still says the retry Consequence worth a follow-up ticketButton labels are now the one text class the design system doesn't govern: they render stock tv-material3 Still open from the previous pass
Refining 3 with the actual lookup order, since my earlier wording overstated it: tv-material3's None of the above blocks merge in my view — 2 is the one to verify on device. Not coveredGradle isn't available in this environment, so |
5491d28 to
e41cb49
Compare
|
Claude finished @sztomek's task in 4m 26s —— View job Code review — incremental (new push)
Two new commits, and they close two of the five open findings. Note the base branch also moved under this PR (
|
| # | Item | Status |
|---|---|---|
| 2 | The three OutlinedButtons with no explicit border (TvFeaturedTile.kt:153,159, TvHomeScreen.kt:105) now derive theirs from colorScheme.border = backgroundActive20 instead of the stock tv-material3 outline |
unchanged — the one to eyeball on device |
| 3 | Duplicate values across ColorScheme slots (backgroundOverlay as both secondaryContainer and surfaceVariant, etc.) make contentColorFor() resolution depend on the library's when order |
unchanged (TvTheme.kt:53-77) |
| — | bodyLarge = caption1 is still the app-wide default LocalTextStyle, but no call site reaches it (everything styleless is inside a button, which pushes stock labelLarge) — forward-looking only, and button labels remain the one text class the design system doesn't govern |
as before; a design call, not a regression |
Nothing here blocks merge in my view. The two new commits are both strict improvements, and the typography reduction is provably parity-preserving.
Not covered
Gradle isn't available in this environment, so :tv:compileDebugKotlin and spotlessCheck are unverified here and rest on CI. The tv-material3 sources aren't in this checkout either, so the contentColorFor slot order and the stock OutlinedButton border default in findings 2–3 are inferred from the library's API rather than read.
• branch feat/tv-theme-provider-prototype
3809384 to
fc3bc34
Compare
e41cb49 to
aea97e0
Compare
Code review — incremental (new push)
|
aea97e0 to
e871009
Compare
|
Claude finished @sztomek's task in 4m 29s —— View job Code review — incremental (new push)
What changed in this push: nothing, in substanceThis trigger and my previous pass (13:27 UTC) cover the same tree. The rebase onto Re-verified against
|
| token | main |
this branch |
|---|---|---|
body |
16.67sp / 21.33sp / w500 | 19.33sp / 24sp / w400 |
title1–callout, caption1, caption2 |
FontWeight(500) |
FontWeight(510) |
| KDoc | "Figma's Google Sans family… FontWeight 500 (400 for Subtitle1)" |
"Figma's SF Pro family… FontWeight 510" |
Sizes, line heights, letterSpacing, includeFontPadding and subtitle1 at w400 are otherwise identical, so this is isolated to the ramp. body has visible fallout at 9 call sites (TvToast.kt:101, TvCreateAccountModal.kt:74,138, TvCreateAccountScreen.kt:72,90, TvSignInQrContent.kt:105, TvSignInScreen.kt:126, TvDownloadAppModal.kt:63,77) — ~16 % larger and lighter. 500 → 510 is subtler: on API 28+ FontWeight goes through Typeface.create(family, weight, italic), so with a variable system font it renders marginally heavier; below 28 both sit under the ≥600 bold cutoff and are indistinguishable.
Worth noting for whoever decides: on main, Body and Caption1 are byte-identical (both 16.67/21.33/w500), which is plausibly why an earlier iteration had body at 19.33/w400. So either main's ramp collapsed two Figma steps, or this branch is a stale copy. For a PR that describes itself as pure plumbing ("names preserved, now lowerCamel"), matching main is the safe move — and if main's ramp is wrong, fix it there so both agree. Either way the KDoc wording fix that's on main shouldn't be reverted here. Fix this →
2. TvScreenBackgroundBrush is a PascalCase @Composable get() (theme/TvScreenBackgroundBrush.kt:9-17)
This reintroduces the exact shape that aea97e0 removed from TvModal. On main it was a plain top-level val, so the conversion is new here. PascalCase is reserved for composables that emit UI; the two other new accessors in this PR (tvColors, tvTypography) follow that. It also builds a fresh Brush.horizontalGradient per read instead of the single app-wide instance — LinearGradient implements equals, so Modifier.background won't re-apply spuriously; it's allocation churn at TvDetailOverlay.kt:80 and TvScaffold.kt:124, not a correctness bug. @ReadOnlyComposable rules out remember, so the fix is structural: either a screenBackground: Brush member of TvColorScheme (computed once per scheme, but inherits the copy() caveat below) or @Composable fun tvScreenBackgroundBrush(): Brush.
3. TvColorScheme lost the KDoc that TvColors carried (theme/TvColorScheme.kt:7)
TvTypography kept its equivalent, so this is the one file whose docs didn't come across. The *Active sentence is the only written record of the naming convention, and the "defined even where a token has no call site yet" note is what stops someone pruning textTertiaryActive / textDisabledActive as dead code — they still have zero call sites.
4. Derived defaults don't survive copy() (theme/TvColorScheme.kt:24-26)
TvColorScheme().copy(backgroundSunken = Color.Red) leaves overlayContainer / translucentOverlayContainer / overlayBorder deriving from the old value. Only one construction site today (TvTheme.kt:15), but copy() is the point of a data class, so it's a quiet trap for whoever first themes a variant.
5. Three OutlinedButtons have no explicit border (TvFeaturedTile.kt:153,159, TvHomeScreen.kt:125)
They now derive it from colorScheme.border = backgroundActive20 (20 % white) instead of the stock tv-material3 outline. Still the one to eyeball on device — add the featured row to the testing instructions.
6. Duplicate values make contentColorFor() resolution library-order-dependent (theme/TvTheme.kt:53-77)
backgroundOverlay is both secondaryContainer (→ textPrimary) and surfaceVariant (→ textSecondary); backgroundActive fills four slots; backgroundBase two. It resolves correctly today (tv-material3 checks secondaryContainer first), but onSurfaceVariant = textSecondary is unreachable and correctness rests on a when order in the library. Filling only the slots you need is the more durable shape.
7. HighlightBrush is the last non-tokenized overlay colour (TvModal.kt:125-130)
Raw Color.White.copy(alpha = 0.08f), now sitting next to three tokenized overlay colours. Pre-existing, but it's the obvious remaining candidate.
Assessment
Findings 2 and 3 are new in the rebase, 1 is the substantive one, and 4–7 are polish. Everything mechanical checks out — the colour migration is provably 1:1 and the typography migration would be too once the ramp is re-synced. Nothing here is a merge blocker except 1, which I'd want resolved (or explicitly decided) since it silently changes 9 texts in a PR that presents itself as pure plumbing.
Not covered
Gradle isn't in this environment's allowed commands, so :tv:compileDebugKotlin and spotlessCheck are unverified here and rest on CI. The tv-material3 sources aren't in this checkout, so the contentColorFor slot order and the stock OutlinedButton border default in 5–6 are inferred from the library's API rather than read. I also couldn't query the PR timeline for the previous head SHA, so "no content change in this push" is inferred from file contents and line numbers matching my previous pass, not from a SHA-to-SHA diff.
• branch feat/tv-theme-provider-prototype
e871009 to
ce87594
Compare
|
Claude finished @sztomek's task in 7m 12s —— View job Code review — incremental (new push)
What changed in this pushOne thing, and it's a doc-only edit: Everything else in the tree is byte-for-byte what I reviewed at 13:28 UTC. Net effect: this makes finding 1 slightly worse rather than better. Removing the sentence removes the only written record of why the ramp uses a synthetic weight, while Re-verified from scratch against
|
| # | Item | Where | Status |
|---|---|---|---|
| 1 | Ramp values behind the merged base: body 19.33/24/w400 vs main's 16.67/21.33/w500; FontWeight(510) vs 500 on 7 tokens; KDoc now silent on both |
theme/TvTypography.kt:9-12, :57-63 |
worse this push — doc removed, values unchanged |
| 2 | TvScreenBackgroundBrush is a PascalCase @Composable get() building a fresh Brush per read; was a plain top-level val on main |
theme/TvScreenBackgroundBrush.kt:9-17 |
unchanged |
| 3 | TvColorScheme lost the KDoc TvColors carried — including the *Active naming convention and the "defined even where unused" note. Six fields have zero call sites: textTertiary, textDisabled, textTertiaryActive, textDisabledActive, backgroundSurface, backgroundActive50 |
theme/TvColorScheme.kt:7 |
unchanged, and now mirrored in TvTypography |
| 4 | Derived defaults don't survive copy() — TvColorScheme().copy(backgroundSunken = X) leaves the three overlay colours deriving from the old value |
theme/TvColorScheme.kt:24-26 |
unchanged |
| 5 | Three OutlinedButtons with no explicit border now derive it from colorScheme.border = backgroundActive20 instead of the stock tv-material3 outline |
TvFeaturedTile.kt:153,159, TvHomeScreen.kt:125 |
unchanged — the one to eyeball on device |
| 6 | Duplicate values across scheme slots make contentColorFor() resolution depend on the library's when order; onSurfaceVariant = textSecondary is unreachable |
theme/TvTheme.kt:53-77 |
unchanged |
| 7 | HighlightBrush is the last non-tokenized overlay colour — raw Color.White.copy(alpha = 0.08f) next to three tokenized ones |
TvModal.kt:125-130 |
unchanged |
Also still true, and worth a follow-up ticket rather than a change here: bodyLarge = caption1 sets the app-wide default LocalTextStyle, but no call site reaches it — everything styleless sits inside a button, which pushes stock labelLarge. So button labels remain the one text class the design system doesn't govern.
Assessment
The mechanical migration is provably parity-preserving on colours, and would be on typography too once the ramp is re-synced. Finding 1 is the only one I'd treat as blocking: a PR that describes itself as "names preserved, now lowerCamel" silently changes 9 texts, and this push removed the doc that was the last hint the divergence was deliberate. Either re-sync with main or split the revalue into its own commit with the reasoning stated. 2-7 are polish.
Not covered
Gradle isn't in this environment's allowed commands, so :tv:compileDebugKotlin and spotlessCheck are unverified here and rest on CI. gh api isn't available either, so I couldn't diff this push against the previous head by SHA — the "doc-only change" conclusion is inferred from file contents and line offsets. The tv-material3 sources aren't in this checkout, so the contentColorFor slot order and the stock OutlinedButton border default in 5-6 are inferred from the library's API rather than read.
• branch feat/tv-theme-provider-prototype
3 participants
Description
Moves the TV design tokens out of static singletons (
TvColors,TvTextStyles) and behind a proper theme provider, so tokens are delivered through the composition instead of referenced as global constants.Design (hybrid, mirroring how the phone app and wear already theme):
TvColorScheme/TvTypography—@Immutabledata classes holding the Figma tokens (names preserved, now lowerCamel), provided viastaticCompositionLocalOffrom a new rootTvThemecomposable.MaterialTheme.tvColors.textPrimary/MaterialTheme.tvTypography.caption2via extension properties onandroidx.tv.material3.MaterialTheme— the same idiom as the phone app'sMaterialTheme.theme.colors….TvThemealso derives a tv-material3ColorScheme+Typographyfrom the tokens (the analogue of the phone'sbuildMaterialColorsshim), so bare tv-material3 components,contentColorFor, and previews get sensible defaults instead of stock Material dark.TvThemewrapsAppTheme(EXTRA_DARK)internally, keeping the sharedLocalColorsavailable (folder colors inTvFolderCard). Root wiring moved toTvActivity; the oldAppTheme { MaterialTheme { … } }wrapper is gone fromTvOnboardingNavHost, and all previews collapsed to a singleTvTheme { }wrapper.Testing Instructions
tvdebug build on an Android TV device/emulator.TvTilePreview,TvTabBarPreview, …) — they should render with the design-system colors under the singleTvThemewrapper.Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xmlI have tested any UI changes...