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 Move Top/Bottom to playlist #4604

Closed

Conversation

ArthurKun21
Copy link

Add Move Top/Bottom to playlist

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #4603

Description

This adds another button to move to Top/Bottom of the playlist

Screenshots

electron_xwa9V0JBLI

Testing

  1. Add at least 3 videos to the playlist
  2. Go to the 2nd Video, click on the Move Video Top and it should move to the Top of the playlist
  3. Go to the 2nd Video, click on the Move Video Bottom and it should move to the Bottom of the playlist

Desktop

  • OS: Window
  • OS Version: Windows 11
  • FreeTube version: 0.19.1

Additional context

This is my first time messing around with javascript/vue. Please forgive some errors if there is any.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 28, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 28, 2024 03:25
@kommunarr
Copy link
Collaborator

kommunarr commented Jan 28, 2024

Thanks for the initiative on this @ArthurKun21! My suggestions (haven't looked into the code yet):

  1. Change labels to "Move Video to Top" and "Move Video to Bottom".
  2. Move "Move Video to Top" arrow to the right of the "Move Video Up/Down" buttons, such that it's next to its counterpart.
  3. I like the icons, but I worry deciphering the two arrow icon sets' different meanings could be confusing to some. If you don't mind, could you try replacing these new icons with arrow-up-long and arrow-down-long, and possibly even up-long and down-long, and take screenshots of the result? I'm interested to see if it might be clearer from a distance.

Thanks @ArthurKun21!

auto-merge was automatically disabled January 28, 2024 04:29

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 28, 2024 04:30
@kommunarr
Copy link
Collaborator

Just had an idea: changing the original up/down to angle-up and angle-down, and making these new icons angles-up and angles-down.

auto-merge was automatically disabled January 28, 2024 04:58

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 28, 2024 04:58
auto-merge was automatically disabled January 28, 2024 05:18

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 28, 2024 05:18
auto-merge was automatically disabled January 28, 2024 05:26

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 28, 2024 05:26
@ArthurKun21
Copy link
Author

electron_QGCXYxidv4

Top -> Angles-up
Up -> Angle-up
Down -> Angle-down
Bottom -> Angles-down

@kommunarr
Copy link
Collaborator

Left a handful of comments after looking into the code. Thanks @ArthurKun21!

auto-merge was automatically disabled January 28, 2024 06:00

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 28, 2024 06:00
@ArthurKun21
Copy link
Author

electron_hBHsvA6Cy5

Forgot to add the new screenshot

@PikachuEXE
Copy link
Collaborator

I will have to review this after #4597 is merged which introduced pagination for user playlists and test on playlists with > 100 items

@absidue
Copy link
Member

absidue commented Jan 28, 2024

The video thumbnail looks really crowded now. As move to top and move to bottom are presumably used a lot less often than the normal up and down, maybe these two new options should be added to the 3 dots menu?

auto-merge was automatically disabled January 28, 2024 15:11

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 28, 2024 15:11
auto-merge was automatically disabled January 28, 2024 15:23

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 28, 2024 15:23
@ArthurKun21
Copy link
Author

dropdown

Remove from the video thumbnail and move options to the dropdown

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

I noticed a weird glitch in the animation

VirtualBoxVM_7OElD5orOG.mp4

Also i agree that the thumbnail is too crowded but im not sure if the placement into the 3 dots menu is the right thing to do here. All the playlist related features are accessible in the playlist it self and this one is now a bit tucked away so i would never look in there to find extra options related to playlists

Im not sure what the correct placement is, open for suggestions/discussions

@ArthurKun21
Copy link
Author

Does this playlist have duplicate videos?

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Feb 1, 2024
Copy link
Contributor

github-actions bot commented Feb 1, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

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

I already deleted that playlist but i created a new one without any duplicates an its still doing it.

VirtualBoxVM_4sypDgz0Gc.mp4

Copy link
Contributor

github-actions bot commented Mar 1, 2024

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

Copy link
Contributor

This PR was closed because it has been stalled for 14 days with no activity.

@github-actions github-actions bot closed this Mar 16, 2024
auto-merge was automatically disabled March 16, 2024 01:42

Pull request was closed

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.

[Feature Request]: Add Move Top/Bottom in the playlist
5 participants