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

Add auto scroll to discover featured carousal #818

Merged
merged 8 commits into from
Mar 10, 2023

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Mar 7, 2023

Description

This adds auto scroll to discover featured carousal

Testing Instructions

No auto scroll on default

  1. Launch the app
  2. Open Discover
  3. Notice the Featured Carousel doesn't auto scroll

Auto Scroll

  1. Enable DISCOVER_FEATURED_AUTO_SCROLL feature flag in base.gradle
  2. Open Discover
  3. Watch the Featured carousel auto scroll
  4. Watch that on the last item the carousel returns to the first item

Auto scroll and manual scroll

  1. Swipe to scroll the carousel forward or backwards
  2. See the autoscroll continues to the next item as expected

Additionally, test that the auto-scroll timer doesn't keep running when the app is backgrounded or moving to another screen.

Screenshots or Screencast

device-2023-03-07-182325.mp4

@ashiagr ashiagr requested a review from a team as a code owner March 7, 2023 12:56
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 and works well! I have a couple of UI/UX thoughts here, apart from the first one they're somewhat my personal preferences, so I'm sharing them more as food for thought for you and @adamzelinski . They're certainly not blockers.

1. "Flashing" images

When the discover carousel is loaded the first time, the images load as they are swiped into view, which creates a bit of a "flash" that I find jarring. It would be good to preload at least the next image now that we're animating the carousel, so it is already loaded when it comes into view.

flashing_images_trimmed.mp4

2. Animation Speed

The animation from one item to the next feels a bit fast and abrupt and distracting to me. Maybe slowing it down and/or using a different kind of easing would help. This may just be my personal preference though.

3. Auto scroll after manually swiping the list

It felt to me like the animation picked back up a bit too quickly for my taste after I swiped the list. I wonder if it would be better to either give a longer delay after the user manually swipes on the list, or maybe even to turn off the automatic scrolling entirely. 🤔

4. Scrolling back to the first item

Feels like it might be nice if instead of scrolling back through all the items at the end, it instead still animated "forward", with the next item being the first. I found this pretty tricky to get working well on the Plus Upgrade screens I did for onboarding, so although I'm sure you could accomplish this, I suspect it may not be worth the effort.

@ghost
Copy link

ghost commented Mar 8, 2023

This is looking really nice thanks @ashiagr and thanks for the great feedback @mchowning
1. Flashing images
I agree with this feedback. It would be great to preload the next image so it doesn't flicker in when the auto scroll begins.
2. Animation speed
I also agree with this, I'm not sure if it's the screen recording or not but it does seem a little quick and a bit harsh with the easing. What intervals do we currently have it set at? Overall, I think the timing could be a little longer for all, and maybe even the first slide a little longer than the rest.
3. Auto-scroll after manual swipe
I think just a longer delay after manually swiping is the way to go here.
4. Scrolling back to the first item
Yes, I agree that at the end of the list instead of scrolling back through all the items to the start it should just keep scrolling "forward" to the first item in the list.

Copy link
Contributor Author

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

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

Hey @mchowning, @adamzelinski 👋

I've tried to address your feedback and left comments mentioning revised prefetch count or durations. Let me know if any of those need to be adjusted.

I'll address scrolling back to the first item animation separately. For now, I've matched it with the iOS app which switches from last to first without any animation.

device-2023-03-08-163930.mp4

@@ -73,6 +79,7 @@ import au.com.shiftyjelly.pocketcasts.ui.R as UR
private const val MAX_ROWS_SMALL_LIST = 20
private const val CURRENT_PAGE = "current_page"
private const val TOTAL_PAGES = "total_pages"
private const val INITIAL_PREFETCH_COUNT = 3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Increased prefetch count from 1 to 3. It might still not be sufficient on a slower network. Let me know if I should increase it.

Copy link
Contributor

@mchowning mchowning Mar 8, 2023

Choose a reason for hiding this comment

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

The flicker might be less frequenty, but I'm definitely still seeing it. I'm at home on a broadband wifi network, so it doesn't feel like network speed should be the issue. Not sure what's going on here because it feels like increasing the prefetch should have fixed 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.

I could see the flickering more noticeably on a manual swipe as compared to auto-scroll. On checking LinearLayoutManager documentation:

By default, {@code LinearLayoutManager} lays out 1 extra page of items while smooth
scrolling, in the direction of the scroll, and no extra space is laid out in all other
situations. You can override this method to implement your own custom pre-cache logic.

protected void calculateExtraLayoutSpace(...)

I've overridden this method in 0fd13a5 and provided an extra page space so that it works for a manual swipe as well. This shouldn't cause too much overhead as an extra page should already have been laid out in case of smooth scrolling. It seems to have improved pre-caching behavior.

Screen.Recording.2023-03-10.at.1.37.33.PM.mov

Feel free to drop any feedback and I'll take it up separately.

@mchowning
Copy link
Contributor

Updates look good. I did leave a few UI comments, but nothing I think you need to change unless Adam suggests a change. Great work on this @ashiagr !

@ashiagr ashiagr enabled auto-merge March 10, 2023 08:28
@ashiagr ashiagr merged commit f6802e1 into main Mar 10, 2023
@ashiagr ashiagr deleted the task/add-auto-scroll-discover-carousal branch March 10, 2023 08:33
@mchowning mchowning mentioned this pull request Mar 29, 2023
8 tasks
@ashiagr ashiagr added this to the 7.35 milestone Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants