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: Add stories datasource + gestures #486

Merged
merged 10 commits into from
Oct 26, 2022

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Oct 25, 2022

📘 Project: #410

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

Description

This PR adds a data source to populate the Stories view with stories and also adds a few gestures.
Currently, it just updates the background color of the StoryView. Different story views will be added in future PRs.

story_datasource_and_gestures.mp4

To test

  1. Set END_OF_YEAR_ENABLED feature flag to true in base.gradle
  2. Run the app
  3. When the prompt appears, tap "View My 2022"
  4. ✅ Check that each story is shown for 2s and that the story indicator (the rectangles at the top) are reflecting the state
  5. ✅ Tap and hold, check that the current story is "paused". The rectangle stop progressing and the story doesn't change
  6. ✅ While seeing the green story, tap the left portion of the screen, you should go back to the magenta one
  7. ✅ While seeing the magenta story, tap the right portion of the screen, you should go to the next story (green)

Loading and error

  1. Return an empty list from EndOfYearStoriesDataSource.loadStories
  2. Run the app
  3. When the prompt appears, tap "View My 2022"
  4. ✅ Check that loading indicator appears while stories are loading (I added a delay to test this)
  5. Wait for a sec
  6. ✅ Notice that error message is displayed when no stories are found.
stories_loading_and_error.mp4

Checklist

  • Should this change be included in the release notes? If yes, please add a line in CHANGELOG.md
  • Have you tested in landscape?
  • Have you tested accessibility with TalkBack?
  • Have you tested in different themes?
  • Does the change work with a large display font?
  • Are all the strings localized?
  • Could you have written any new tests?
  • Did you include Compose previews with any components?

@ashiagr ashiagr requested a review from a team as a code owner October 25, 2022 12:08
@ashiagr ashiagr mentioned this pull request Oct 25, 2022
35 tasks
import au.com.shiftyjelly.pocketcasts.utils.seconds

abstract class Story {
val storyLength: Long = 2.seconds()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Story length is currently the same for all stories. It'll be made dynamic in a future PR.

}
}
},
onLongPress = {
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 used onLongPress tap gesture to pause the story. It takes sometime for the story to pause while the tap qualifies as a long press tap. I can improve this behavior later.

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 is working great. Nice work Ashita!

.pointerInput(Unit) {
detectTapGestures(
onTap = {
if (!isPaused) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't able to find a way to trigger onTap while onPause was true. This obviously isn't doing any harm, but maybe with the press and long press handling we don't need to track the onPause state manually anymore? Or maybe I just didn't find the right way to trigger it. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

onPress gets triggered for both onTap and onLongPress and I want to re-start the timer only when onLongPress is triggered which is why I'm manually tracking onPause state. I'll see if I can improve this behavior as I noted here.

Thank you for finding all these discussion points!


private var currentIndex: Int = 0
private val nextIndex
get() = (currentIndex.plus(1)).coerceAtMost(numOfStories.minus(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping the next index at the last story means that skipping forward on the last story restarts the last story. This might be fine, but I just wanted to mention it because I could see it being a bit confusing to a user. Of course, skipping to the end and having nothing else to play could also be confusing, so I don't really have a specific suggestion here, more just food for thought.

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'd like to keep the current behavior for now. I thought of starting from the beginning for this scenario but if there are several stories, it might not be a pleasant experience going through all stories. And as you mentioned, skipping to the end and having nothing else to play could also be confusing.

@mchowning mchowning merged commit 5cd17ce into main Oct 26, 2022
@mchowning mchowning deleted the task/410-add-stories-datasource branch October 26, 2022 12:13
@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