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

End of Year: Show top 1 podcast and top 5 podcasts #537

Merged
merged 8 commits into from
Nov 4, 2022

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Nov 3, 2022

📘 Project: #410

iOS PR: Automattic/pocket-casts-ios#415

Description

Adds the story for the top 1 podcast and the top 5 podcasts.

PS: Ignore hard-coded strings and placeholder layouts/ design in StoryViews.

To test

Testing Instructions

  1. Set END_OF_YEAR_ENABLED feature flag to true in base.gradle
  2. Run the app and make sure you're logged in to an account with a few items on Listening History
  3. When the prompt appears, tap "View My 2022"
  4. Skip to the 5th story
  5. ✅ Check that it shows your most listened podcast, together with the number of episodes and listening time
  6. Skip to the 6th story
  7. ✅ Check that it shows your top 5 podcasts

Screenshots or Screencast

Top 1 podcast Top 5 podcasts

Checklist

N/A (added placeholder layouts + strings)

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • 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 requested a review from a team as a code owner November 3, 2022 09:58
@ashiagr ashiagr mentioned this pull request Nov 3, 2022
35 tasks
…410-show-top-podcasts

# Conflicts:
#	modules/features/endofyear/src/main/java/au/com/shiftyjelly/pocketcasts/endofyear/stories/EndOfYearStoriesDataSource.kt
@ashiagr ashiagr force-pushed the task/410-show-top-podcasts branch from 988a81e to 376104a Compare November 3, 2022 11:40
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.

Looks good. 👍

While testing this I did notice that there is a bit of a flicker when switching to the stories with 5 list items (happen on both the top podcast and top categories ones). It's quick (you really have to slow down the video to see it), but it is definitely noticeable on my device.

device-2022-11-03-095115.mp4

My guess is that coil is loading the podcast images asynchronously. If it's not easy to delay displaying the screen until the images are loading, perhaps we could just include some kind of placeholder so the text doesn't jump (ideally, the PodcastImage component itself would take care of that, or at least have an option to to take care of that). That would probably be enough to completely hide this. I haven't tested any of that, so all of that might be completely wrong.

I'll go ahead and approve because this doesn't need to be addressed in this PR.

@ashiagr
Copy link
Contributor Author

ashiagr commented Nov 4, 2022

Thanks for reviewing it, @mchowning! I'll address the flicker issue in a future PR.

Base automatically changed from task/410-show-different-categories-listened to main November 4, 2022 06:17
@ashiagr ashiagr enabled auto-merge November 4, 2022 06:17
@ashiagr ashiagr merged commit c57ff73 into main Nov 4, 2022
@ashiagr ashiagr deleted the task/410-show-top-podcasts branch November 4, 2022 06:27
@ashiagr ashiagr added this to the 7.27 ❄️ milestone Nov 24, 2022
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