Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Playback 2023 - Top listened categories story #1482

Merged
merged 25 commits into from
Oct 30, 2023
Merged

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Oct 18, 2023

📘 Part of: #1463 🎨 Designs: lH66LwxxgG8btQ8NrM0ldx-fi-1599_20385

Description

Adds the 2023 visual for the Top Categories Story.

Testing Instructions

  1. Make sure you're logged in to an account that has a few episodes listened
  2. Go to Profile
  3. Tap the EOY image
  4. Skip to the fifth story
  5. ✅ Check that the story looks good, that it shows your top categories and for the top one, the number of episodes listened to
  6. Share this story
  7. ✅ The story image asset should look good

Screenshots or Screencast

Pixel 7 (Normal display) Pixel 7 (Large display)
Nexus 9 Tablet

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@ashiagr ashiagr mentioned this pull request Oct 18, 2023
25 tasks
Base automatically changed from task/1463-top-5-podcast to main October 19, 2023 05:16
This is done to avoid crash in another language which still have two dynamic strings
# Conflicts:
#	modules/features/endofyear/src/main/java/au/com/shiftyjelly/pocketcasts/endofyear/views/stories/StoryTopPodcastView.kt
#	modules/services/localization/src/main/res/values/strings.xml
#	modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/endofyear/EndOfYearManagerImpl.kt
) {
Image(
painterResource(id = R.drawable.story_blurred_background),
contentDescription = null,
contentScale = ContentScale.Crop,
modifier = Modifier.fillMaxSize()
.alpha(0.6f)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added alpha to the blurred background as those looked brighter than Figma backgrounds. This has also improved the readability of the title on the "Top Podcast" story. Let me know if you have other ideas.

Before After

@ashiagr ashiagr marked this pull request as ready for review October 20, 2023 10:46
@ashiagr ashiagr requested a review from a team as a code owner October 20, 2023 10:46

private const val BackgroundColor = 0xFF744F9D
private val CategoryColor = Color(0xFF686C74)
private val CategoryFontSize = 40.sp
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reduced the font size for the categories so that they fit well on different display sizes and on tablets.

@@ -1592,7 +1600,7 @@
<!-- Text that appear when someone share the listened numbers story to Twitter. %1$d is a placeholder for the number of podcasts listened and %2$d for the number of episodes. -->
<string name="end_of_year_story_listened_to_numbers_share_text">I listened to %1$d different podcasts and %2$d episodes in 2022</string>
<!-- Title for the story that display the most listened podcast by the user this year. %1$s is a placeholder for the podcast title and %2$s is a placeholder for the author. -->
<string name="end_of_year_story_top_podcast">%1$s was your most listened show in 2023</string>
<string name="end_of_year_story_top_podcast_title">%1$s was your most listened show in 2023</string>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful in reviewing string updates as the older strings may have different placeholder counts in translations that could cause a crash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for mentioning this. I wouldn't have thought about it, and now I'll be more careful.

Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This screen is looking really sharp! Nice job!

@@ -338,7 +338,9 @@ abstract class EpisodeDao {

@Query(
"""
SELECT DISTINCT podcast_episodes.uuid as episodeId, COUNT(DISTINCT podcast_id) as numberOfPodcasts, SUM(played_up_to) as totalPlayedTime,
SELECT DISTINCT podcast_episodes.uuid as episodeId, COUNT(DISTINCT podcast_id) as numberOfPodcasts,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, but I think it would help readability to put the episodeId and numberOfPodcasts on separate lines like we have with the other parameters. Same with the mostListenedPodcastId and mostListenedPodcastTintColor on line 345.

Certainly not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, updated in 5a0363c

@ashiagr ashiagr enabled auto-merge (squash) October 30, 2023 03:48
@ashiagr ashiagr merged commit 5dac510 into main Oct 30, 2023
@ashiagr ashiagr deleted the task/1463-top-categories branch October 30, 2023 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants