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 Play All and Shuffle Button #6153

Open
wants to merge 9 commits into
base: development
Choose a base branch
from

Conversation

JL0000
Copy link

@JL0000 JL0000 commented Nov 12, 2024

Add Play All and Shuffle Button

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

#5618

Description

Added Play all and Shuffle buttons to both user and youtube playlists.
Clicking on Play all always plays the first video in the playlist.
Clicking on Shuffle plays a random video in the playlist and toggles shuffle mode on.

Screenshots

Before

User Playlist
playlist-no-button
playlist-no-button-list
Youtube Playlist
yt-playlist-no-button

After

User Playlist
playlist-button
playlist-button-list
Youtube Playlist
yt-playlist-button

Testing

Play all plays the first video after video order is changed.
Shuffle works as intended after a video is deleted whilst playing through the playlist.
Shuffle resets the playlist after adding a new video whilst it is playing its last item in the iteration. This behaviours is inherited from the development branch.

Desktop

  • OS: MacOs
  • OS Version: 15.1
  • FreeTube version: v0.22.0 Beta

Additional context

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 12, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 12, 2024 16:44
Copy link
Member

Choose a reason for hiding this comment

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

Everything looks good but i did notice this error when entering an empty playlist.

VirtualBoxVM_l8o7eOKNHE.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 15, 2024
auto-merge was automatically disabled November 15, 2024 11:48

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 15, 2024 11:49
@JL0000
Copy link
Author

JL0000 commented Nov 15, 2024

I have fixed this error and I have hid the buttons when the playlist is empty.

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Nov 15, 2024
@PikachuEXE
Copy link
Collaborator

I don't think the buttons should be shown in edit mode
Screenshot 2024-11-17 at 08 10 16

I will fix other buttons/inputs myself later

auto-merge was automatically disabled November 21, 2024 16:49

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 21, 2024 16:50
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

suggestion (non-blocking): I'd ideally like it if on a desktop view breakpoint for the grid view playlist page, the top row became a flex container with the icons on the left and the new buttons on the right. This way the fixed container wouldn't have to be as tall.

discussion: I do also think that all of our icons should have visible labels, not just these two, for accessibility & ease of use reasons. That said, that's no reason to not have them here, just that in the future I recommend that all of these buttons become of the same type, with optionally the same positioning differences kept if we feel like it.

Screenshot_20241123_083315

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

I do also think that all of our icons should have visible labels, not just these two, for accessibility & ease of use reasons. That said, that's no reason to not have them here, just that in the future I recommend that all of these buttons become of the same type, with optionally the same positioning differences kept if we feel like it.

This is a signal boost for #4950 and i like the member to chime in on that PR. I think it would be a good improvement.

@JL0000
Copy link
Author

JL0000 commented Nov 23, 2024

suggestion (non-blocking): I'd ideally like it if on a desktop view breakpoint for the grid view playlist page, the top row became a flex container with the icons on the left and the new buttons on the right. This way the fixed container wouldn't have to be as tall.

So I tried this out and it turned out fine
Screenshot 2024-11-23 at 21 57 12
On a public playlist, because playlistOptionsWrapper was moved out of channelShareWrapper (the new buttons in the playlistOptionsWrapper will push playlistChannel to the left), the copy and share buttons will not be to the immediate right of the channel name. Is this fine?
Screenshot 2024-11-23 at 21 54 58

auto-merge was automatically disabled November 25, 2024 17:55

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 25, 2024 17:55
auto-merge was automatically disabled November 25, 2024 18:06

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 25, 2024 18:06
Copy link
Member

Choose a reason for hiding this comment

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

Getting this error when opening up FT

image

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 25, 2024
auto-merge was automatically disabled November 25, 2024 22:55

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 25, 2024 22:55
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Nov 25, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc

Thoughts:

Playlist Grid view: To it seems a bit weird that the Secondary color theme colors are divided by the red icons. Maybe place the buttons to the left of the bookmark icon so that its grouped by color?

image

Channel playlist view: Would maybe look nicer if the buttons are placed to the left of the copy and share link buttons

image

Playlist List view: The suggestion above cant be applied here as there isnt space for it. I do think that it kind of looks off when sitting between the icons above and the searchbar but im not sure if it can be made to look 'nicer'

image

@JL0000
Copy link
Author

JL0000 commented Nov 26, 2024

I tried a few things and I think the best it looked was grid mode not changing but list mode having centered buttons.
Screenshot 2024-11-26 at 19 35 54 Screenshot 2024-11-26 at 19 36 09

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: waiting for review For PRs that are complete, tested, and ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add Shuffle button within both YouTube and User Playlists
4 participants