Show no of playlist a video already added to on add playlist button
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
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
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.
15px looks like this:
Seems way too big for me
12px?
12px?
I think you're right. It commands too much attention at a larger size.
Feedback after testing:
- It's still on the right side for RTL languages. Please fix:
- 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.
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.
- 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.
RTL now looks like:
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)
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
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.
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
Removed counter in user playlist only (still present in remote playlist)
@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
This pull request has conflicts, please resolve those before we can evaluate the pull request.
Conflicts have been resolved. A maintainer will review the pull request shortly.
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)
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.
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
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.
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)
I'd be open to having it in experimental settings, but I'd wait to see what others think before going forward with that
This pull request has conflicts, please resolve those before we can evaluate the pull request.
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.
Will revisit after Vue 3 migration done