[TV] Show a download app modal when creating a playlist - #5658
Conversation
|
Claude finished @sztomek's task in 1m 31s —— View job Code ReviewReviewed the diff against
What looks good
Minor suggestions (non-blocking)
Verification
|
| ) | ||
| Image( | ||
| painter = rememberQrPainter(content = DOWNLOAD_URL, size = QrCodeSize), | ||
| contentDescription = stringResource(LR.string.tv_playlists_download_subtitle), |
There was a problem hiding this comment.
Minor (accessibility): the QR image's contentDescription reuses the subtitle string, so TalkBack will read "They'll be waiting here when you're done. Scan to get the app" twice — once for the subtitle Text above and again for the image. Consider a QR-specific description (e.g. a dedicated string like "QR code linking to pocketcasts.com/downloads") or null if the adjacent URL text makes it redundant.
| private const val DOWNLOAD_URL = "https://www.pocketcasts.com/downloads" | ||
| private const val DOWNLOAD_URL_LABEL = "pocketcasts.com/downloads" |
There was a problem hiding this comment.
DOWNLOAD_URL_LABEL is maintained as a second literal independent of DOWNLOAD_URL, so the QR target and the visible label can silently drift apart if one is edited. TvSignInScreen derives the label from the URL (removePrefix("https://").removePrefix("http://").trimEnd('/')). Since the label here also drops the www., that exact helper won't match, but deriving it (e.g. .removePrefix("https://www.").trimEnd('/')) would keep the two in sync. Not blocking — just flagging the duplication.
553e23d to
95fae55
Compare
|
Claude finished @sztomek's task in 1m 13s —— View job Code Review
Reviewed against What looks good
Minor (non-blocking) — both still open from the earlier pass
Other notes
Nice work — good to go once you decide on the two minor items above. |
82792f4 to
7940fbb
Compare
95fae55 to
756d773
Compare
|
Claude finished @sztomek's task in 44s —— View job Code Review
Re-reviewed against
What looks good
Remaining note (non-blocking)
No bugs, security, or performance concerns. Nice work — this is good to go. 🚀 |
geekygecko
left a comment
There was a problem hiding this comment.
Looks good to me. I only had some minor comments.
7940fbb to
7bd98d2
Compare
756d773 to
efc46be
Compare
|
Claude finished @sztomek's task in 3m 1s —— View job Code Review
Re-reviewed against What changed since the last pass
Verified correct
Inline comments
Other observations (no action needed)
Looks good to me — ship it once you've decided on the two inline nits. 🚀 |
| val ModalTitle = TextStyle( | ||
| fontSize = 24.sp, | ||
| fontWeight = FontWeight.SemiBold, | ||
| lineHeight = 28.sp, | ||
| textAlign = TextAlign.Center, | ||
| platformStyle = PlatformTextStyle(includeFontPadding = false), | ||
| ) | ||
|
|
||
| val ModalBody = TextStyle( | ||
| fontSize = 16.sp, | ||
| lineHeight = 20.sp, | ||
| textAlign = TextAlign.Center, | ||
| platformStyle = PlatformTextStyle(includeFontPadding = false), | ||
| ) |
There was a problem hiding this comment.
Following up on @geekygecko's Figma question: these two are exact duplicates of styles already in this file apart from textAlign.
ModalTitle(24sp / SemiBold / 28sp) ==PlaylistCardTitleModalBody(16sp / 20sp) ==FeaturedTileDescription
The Figma screenshot names the title Title 3 · 48/56, which is exactly 24/28 at 1x — so the values already match a shared type-scale entry, only the name is component-scoped. Renaming these to the Figma scale (e.g. Title3, Body) and setting textAlign at the call site (Text(textAlign = TextAlign.Center, …), as TvEmptyState already does) would collapse four near-identical styles into two reusable ones and make the next screen that needs the same type pick them up for free.
Non-blocking — but if you'd rather keep the component-scoped naming, worth saying so on the review thread so it doesn't get re-raised. Fix this →
| .padding(top = 16.dp) | ||
| .focusRequester(focusRequester), | ||
| ) { | ||
| Text(stringResource(LR.string.done)) |
There was a problem hiding this comment.
Minor consistency: TvProfileModalButton styles its label with TvTextStyles.ModalButtonLabel (17sp, weight 510), but this Done label falls back to the default TV Material typography. Two modals side by side will render their buttons with different type. Same applies to TvEmptyState's Text(actionLabel) now that it's a filled button.
| Text(stringResource(LR.string.done)) | |
| Text(stringResource(LR.string.done), style = TvTextStyles.ModalButtonLabel) |
7bd98d2 to
15f1088
Compare
efc46be to
7e3afb6
Compare
|
Claude finished @sztomek's task in 1m 41s —— View job Code Review
Re-reviewed against This synchronize was a rebase — no content changeAll six blob SHAs are byte-identical to the state I reviewed at 11:20 ( I re-verified that the rebase didn't break anything the change depends on:
Still open from the last pass (both non-blocking)
One small addition
No bugs, security, or performance concerns. Everything from the earlier reviews (state hoisting via I did not run a build or Still good to go once you've decided on the two inline nits. 🚀 |
Description
Implements the Create playlist action on the TV Playlists tab's empty state, mirroring the tvOS
DownloadAppModal: since playlists can't be created on the TV, the button opens a modal with a QR code pointing topocketcasts.com/downloadsso the user can grab the phone app.TvDownloadAppModalreuses the glassyTvModal(widened via a newwidthparameter) with the tvOS copy, a QR code rendered through the existingqrmodule, the plain-text URL, and a focused Done button.Figma: Ftk3KwnfqaK4g57yCN63p0-fi-3246_3451
Fixes PCDROID-694 https://linear.app/a8c/issue/PCDROID-694/create-playlists-modal
Testing Instructions
tvapp with signed out statehttps://www.pocketcasts.com/downloads.Screenshots or Screencast
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xmlI have tested any UI changes...