openzim / youtube

Create a ZIM file from a Youtube channel/username/playlist
GNU General Public License v3.0
39 stars 26 forks source link

Add playlist panel to video player page #237

Closed dan-niles closed 1 week ago

dan-niles commented 2 weeks ago

This PR adds a playlist panel to the video player page.

Screenshot:

image

Closes #216

benoit74 commented 1 week ago

Maybe one question for @Popolechien: we have a setting at ZIM level regarding autoplay, which is used to decide if we want to autoplay the video every time a page is opened. By default, it is off.

However, with the new UI in playlist mode we will now automatically move from one video to the next when the video is over. Do we still want to take into account the autoplay setting? I.e. if autoplay setting is false, when a video finishes, the UI moves automatically to the next video but video does not start. Or do we want to always start the next video automatically in such a situation?

Popolechien commented 1 week ago

What does Youtube do? Unless we can make a compelling argument that our users are different from theirs, we should mimic YT's behaviour as much as possible (which may, at times, include things we at a personal level do not particularly enjoy).

Autoplay it is, then.

benoit74 commented 1 week ago

Agreed.

Does it means we do not really need the "autoplay" setting at scraper level and could even imagine to get rid of it? Do you recall when / why it was introduced? I imagine it could be to save bandwidth (avoid loading the video if the user finally decides he does not want to see it) but I'm not sure it makes lot of sense in terms of usability.

Popolechien commented 1 week ago

Do you recall when / why it was introduced?

I think you wildly overestimate my technical involvement or even my ability to understand why certain technical choices are made. But thank you for that :-)

I am pretty sure I don't understand the difference between autoplay and autoplay at scraper level, so I'll focus on the end result and simply ask that things, well, autoplay.

dan-niles commented 1 week ago

Only a small interrogation. I'm not familiar at all with the nextTick API, and I fail to find examples on the net on how to use this properly. Do we really need the two calls to nextTick, once in the watch and once in the method itself?

@benoit74 Oops that's a mistake. Thanks for pointing it out. I must have duplicated it when doing some debugging and forgot to remove it. Removed it in 59b89cd.

nextTick() is used to ensure that the automatic scrolling in the playlist panel happens after the DOM updates for loading the next video are completed.

dan-niles commented 1 week ago

If there is anything else to fix please let me know or else I can squash and rebase. Should I remove the --autoplay argument and make autoplay always true? or do we create a seperate issue for this?

benoit74 commented 1 week ago

If there is anything else to fix please let me know or else I can squash and rebase.

Please squash and rebase

Should I remove the --autoplay argument and make autoplay always true? or do we create a seperate issue for this?

Please update https://github.com/openzim/youtube/issues/233 by changing title and add a new comment saying that we've finally decided to autoplay: true and remove CLI argument, referring to the discussion in this PR. And do the change in other PR for clarity.

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 1.62%. Comparing base (d0212f9) to head (4af6500).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #237 +/- ## ===================================== Coverage 1.62% 1.62% ===================================== Files 11 11 Lines 1049 1049 Branches 158 158 ===================================== Hits 17 17 Misses 1032 1032 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dan-niles commented 1 week ago

Squash and rebase completed