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

Update watch view playlist component to auto scroll to current video #3399

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Apr 4, 2023

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Fixes #1521

Description

Very strange/inconvenient to me that the playlist is not showing current video in playlist when playing a video in a playlist
This PR makes it auto scroll to current video

Screenshots

Screen.Recording.2023-04-04.at.14.19.14.mov

Testing

Test with both Local & Invidious API

  • Find a playlist (long enough to have middle place)
  • Play videos at the beginning, end & middle
  • Ensure no strange scroll behaviour (besides scrolled to the video entry being played)

Test with different widths, sorting

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

@PikachuEXE PikachuEXE force-pushed the feature/watch-page/playlist-scroll-to-current branch from e19195a to 820001f Compare April 4, 2023 07:11
@PikachuEXE PikachuEXE marked this pull request as ready for review April 4, 2023 07:11
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 4, 2023 07:12
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 4, 2023
@PikachuEXE
Copy link
Collaborator Author

nextTick actually doesn't work for items near the end of a playlist

Copy link
Member

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left a comment

Choose a reason for hiding this comment

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

nextTick actually doesn't work for items near the end of a playlist

LGTM, i dont think its that big of a deal because even if it's not the current video on top of the list, thumbnail of the current video playing is still in view.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

nextTick actually doesn't work for items near the end of a playlist

@PikachuEXE could u explain to me why u reverted in the latest commit to timeout? To me the nextTick didn't felt like it introduced a major issue with the items not working at the end of the playlist. Am i missing something here?

@PikachuEXE
Copy link
Collaborator Author

PikachuEXE commented Apr 18, 2023

With nextTick "thumbnail of the current video playing is still in view" is false
The whole items is out of view
Visit https://youtu.be/vB1djFcT4xQ?list=PLzFTGYa_evXj7dJmh7qXNTPQdSbxattSE without cache (visit that video right after app launch or reload)

(Current is 90)
image

@PikachuEXE
Copy link
Collaborator Author

@absidue @ChunkyProgrammer
Please review when free~
I will try one more day to find non setTimeout solution, but that's it
It's still better than having no auto scroll in terms of UX IMO

@PikachuEXE
Copy link
Collaborator Author

I have following changes but still have setTimeout but with 0 in settings
image
image
image

I think setTimeout is difficult to avoid due to rendering must occur first
See https://stackoverflow.com/a/47636157/838346 for explanation

Let me know if this change is preferred or the current pushed version

@PikachuEXE
Copy link
Collaborator Author

Another way to implement this without setTimeout would be to force 3-4 video items near the current video item to be visible on start, the remaining items can stay not rendered. Tested locally
Code diff:
image

@absidue
Copy link
Member

absidue commented May 5, 2023

Looks good, please use created instead of mounted otherwise that value will be applied after it's already been rendered once, so it wouldn't be the initial value.

@PikachuEXE
Copy link
Collaborator Author

@absidue I assume you want me to go with the latest attempt to remove setTimeout, used created

Additional change:
Fixed issue when Invidious API updated
(watch view becomes "loaded" earlier than Local API)
Testing section updated (test with both APIs)

@FreeTubeBot FreeTubeBot merged commit d3b3be7 into FreeTubeApp:development May 19, 2023
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label May 19, 2023
@PikachuEXE PikachuEXE deleted the feature/watch-page/playlist-scroll-to-current branch May 30, 2023 01:55
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.

Playlist doesnt show current playing video
5 participants