FreeTube icon indicating copy to clipboard operation
FreeTube copied to clipboard

Show no of playlist a video already added to on add playlist button

Open PikachuEXE opened this issue 1 year ago • 16 comments

Pull Request Type

  • [ ] Bugfix
  • [x] Feature Implementation
  • [ ] Documentation
  • [ ] Other

Related issue

https://github.com/FreeTubeApp/FreeTube/issues/4982

Description

Update add to playlist button to show no. of playlists a video already added to

Screenshots

image image image image

Testing

  • Add some videos to playlist(s)
  • Ensure counter shown & correct on add playlist button (in different places)
  • Ensure other icon buttons style unchanged

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

Just using default colors & position for preview Feel free to suggest other styles

When reviewing src/renderer/components/ft-icon-button/ft-icon-button.vue maybe hide whitespace changes for easier review

PikachuEXE avatar May 21 '24 02:05 PikachuEXE

suggestion (blocking): I get why you would want it to be small, but I'm having trouble seeing it in your regular zoom images. If possible, I'd recommend that it's at least 14-16px.

kommunarr avatar May 21 '24 02:05 kommunarr

15px looks like this: image

Seems way too big for me

12px? image image

PikachuEXE avatar May 22 '24 01:05 PikachuEXE

12px?

I think you're right. It commands too much attention at a larger size.

Feedback after testing:

  1. It's still on the right side for RTL languages. Please fix:

Screenshot_20240522_110831

  1. At a bigger font-size, and its position on the right side, it pops out too much now, in that you can't tell if it's popping out of the icon to the right of it.

Screenshot_20240522_110728

Interestingly, this problem is not apparent with the current RTL issue. Therefore, maybe a good fix for 2 is just moving it to the inline-start side.

  1. This might sound minor, but there's some redundancy to it in certain places that feels subtly irksome: i. You're on the Playlist page for the one playlist that has it, but you see the 1 on the icon. This is not conveying any new information. ii. You have it quick bookmarked on any route, and that's the only playlist containing that video, but you see the 1 on the icon. This is not conveying any new information. iii. Intersection of cases i and ii and the Playlist page.

Again, this might sound minor, but it's presenting the same information twice, that adds unnecessary visual scanning and processing time to understand what is being logically conveyed. It also makes this feature unnecessarily prominent for the more common use case of someone who uses their Quick Bookmark playlist for the vast majority of their videos. I'd strongly recommend implementing carveouts for the above three cases.

kommunarr avatar May 22 '24 16:05 kommunarr

RTL now looks like: image

Reduced translate value and list view look like this: (If I guess wrong on what At a bigger font-size means please let me know how to reproduce) image

For (3)

  • The original issue saying there is no indication of a video is saved in any playlist. I agree that there is no need for videos in user playlists to solve that issue. (To be done later)
  • I can make the counter disappeared when it's only saved in quick bookmark playlist. But I think there might be people in the issue wants to see the count if the video is saved in more than 1 playlists (quick bookmark playlist included or not). It's a proxy for "what playlists is the video saved in" request (whether it should be addressed is another thing).
  • Not considering (iii) yet until we agree what changes to be made for (i) * (ii) and implemented

PikachuEXE avatar May 23 '24 02:05 PikachuEXE

Thanks so much @PikachuEXE! Could you switch the default side to be inline-start (or ya know what I mean, the logical equivalent) for the icons?

I can make the counter disappeared when it's only saved in quick bookmark playlist.

Yes, to clarify, that's what I intended with ii. If you're removing it from the User Playlist view altogether, that makes i and iii no longer applicable.

kommunarr avatar May 23 '24 03:05 kommunarr

I am not entirely sure if you mean moving from right to left, but I made the change anyway Fix me if I guess wrong image image

Removed counter in user playlist only (still present in remote playlist) image

PikachuEXE avatar May 23 '24 07:05 PikachuEXE

@absidue I am aware of the performance issue when there are many playlists with many videos I doubt a solution can be developed before next release So maybe we can disable it in places that can take a performance hit And/or disable this only if no. of playlist/saved videos is above some number

PikachuEXE avatar May 24 '24 22:05 PikachuEXE

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

github-actions[bot] avatar May 25 '24 10:05 github-actions[bot]

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar May 26 '24 00:05 github-actions[bot]

It done that way intentionally See original issue and comment in this PR https://github.com/FreeTubeApp/FreeTube/pull/5141#issuecomment-2125245573 (and a few comments afterward)

PikachuEXE avatar May 30 '24 00:05 PikachuEXE

I still have concerns about the performance impact / energy usage. I'd prefer to wait on this until after the Vue 3 migration where we can use Maps / Sets, as it currently doesn't pass my personal cost versus benefit evaluation. I won't block it if the rest of the team feels differently, though.

kommunarr avatar Jun 06 '24 22:06 kommunarr

We can disable this in any video collection view (i.e. only on watch page? And only update implementation & enable it elsewhere after vue 3 migration (which takes no idea how long

PikachuEXE avatar Jun 06 '24 22:06 PikachuEXE

I think the tough thing is that it's not very useful of a feature if people don't know when/where it will appear, and probably more confusing than not in terms of what we're training the users to expect. I know it sucks to suggest putting a PR that you put a good deal of work into into stasis for so long, so I'll just leave it to the rest of the team on how we should handle it.

kommunarr avatar Jun 06 '24 23:06 kommunarr

It's less urgent for me as https://github.com/FreeTubeApp/FreeTube/pull/5044 is merged (I simply open it, found out it's already added, close it) The other way around is to add a setting to only show it when enabled with notes about performance implication (but not sure how this affects performance in vue 3)

PikachuEXE avatar Jun 06 '24 23:06 PikachuEXE

I'd be open to having it in experimental settings, but I'd wait to see what others think before going forward with that

kommunarr avatar Jun 07 '24 02:06 kommunarr

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

github-actions[bot] avatar Jun 15 '24 12:06 github-actions[bot]

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.

github-actions[bot] avatar Jul 16 '24 01:07 github-actions[bot]

Will revisit after Vue 3 migration done

PikachuEXE avatar Jul 16 '24 02:07 PikachuEXE