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

PikachuEXE · 2024-05-21T02:08:54Z

Pull Request Type

Bugfix
Feature Implementation
Documentation
Other

Related issue

#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

kommunarr · 2024-05-21T02:25:31Z

src/renderer/components/ft-icon-button/ft-icon-button.vue

+          fontSize: `${size + padding * 2}px`,
+        }"
+        counter
+        :value="counterValue"


issue (blocking): There's currently no non-visual or semantic indication of the meaning, which makes it inaccessible to screen reader users, and somewhat confusing for people who don't intuitively grasp the meaning.

Would updating the title make it better? (good enough or not is something else)

I think that's a safe way to do it. That label seems pretty good too, albeit maybe a bit too long. Maybe Add to Playlist (Aleady Added to 2)?

kommunarr · 2024-05-21T02:30:05Z

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.

src/renderer/components/ft-playlist-add-video-button/ft-playlist-add-video-button.scss

PikachuEXE · 2024-05-22T01:57:04Z

15px looks like this:

Seems way too big for me

12px?

kommunarr · 2024-05-22T16:32:23Z

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.

* development: Update playlist name with title (FreeTubeApp#5150) User playlists as grid (FreeTubeApp#4949) Add custom webpack loader to remove unused mimetypes from mime-db (FreeTubeApp#5148) ^ Update GH action eps1lon/actions-label-merge-conflict (FreeTubeApp#5034) Translated using Weblate (Italian) Translated using Weblate (Serbian) Translated using Weblate (Estonian) Translated using Weblate (Bulgarian) Translated using Weblate (Spanish) Translated using Weblate (Italian) Translated using Weblate (Polish) Translated using Weblate (Portuguese (Brazil)) Translated using Weblate (French) Translated using Weblate (Chinese (Traditional)) Translated using Weblate (Chinese (Simplified)) Fix gap next to banner when Hide Side Bar Labels is enabled (FreeTubeApp#5120)

PikachuEXE · 2024-05-23T02:53:03Z

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

kommunarr · 2024-05-23T03:05:45Z

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.

PikachuEXE · 2024-05-23T07:26:40Z

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)

kommunarr

This looks great! I was initially skeptical if this requested feature could be implemented well, but the way you've implemented it solves the problem while also not overwhelming the user with information, or affecting other use cases negatively.

Fix the minor linting issue below please.

Edit: The performance implications mentioned in the Matrix channel are notably concerning. This will need to be addressed in some way for this to not cause a noticeable downgrade on the user experience.

src/renderer/components/watch-video-info/watch-video-info.js

PikachuEXE · 2024-05-24T22:56:23Z

@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

github-actions · 2024-05-25T10:32:55Z

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

absidue

Here are a few suggestions to improve the performance, you will want to merge in development or rebase this branch on development, first though.

src/renderer/components/ft-playlist-add-video-button/ft-playlist-add-video-button.js

src/renderer/components/ft-list-video/ft-list-video.js

* development: Fix channel sort values to show the values they are (FreeTubeApp#5162) Translated using Weblate (Russian) Translated using Weblate (French) Quick bookmark button RTL & hover fixes (FreeTubeApp#5157) Use addVideo instead of addVideos for quick bookmark button (FreeTubeApp#5168) Cache quick bookmark playlist to reduce the amount of lookups (FreeTubeApp#5169) Translated using Weblate (Arabic) Translated using Weblate (Indonesian) Fix hide/show channel in ft-list-video (FreeTubeApp#5149) # Conflicts: # src/renderer/components/ft-list-video/ft-list-video.js # src/renderer/components/watch-video-info/watch-video-info.js

github-actions · 2024-05-26T00:38:14Z

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

absidue

As we already check if the video is in the quick bookmark playlist inside the parent component, we can just pass down the result, instead of needing to perform the check again inside this component

src/renderer/components/ft-playlist-add-video-button/ft-playlist-add-video-button.js

src/renderer/components/watch-video-info/watch-video-info.vue

src/renderer/components/ft-list-video/ft-list-video.vue

src/renderer/components/ft-playlist-add-video-button/ft-playlist-add-video-button.js

efb4f5ff-1298-471a-8973-3d47447115dc

The only place im missing this number on a thumbnail is inside of a playlist

PikachuEXE · 2024-05-30T00:03:17Z

It done that way intentionally
See original issue and comment in this PR #5141 (comment) (and a few comments afterward)

kommunarr · 2024-06-06T22:37:06Z

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.

PikachuEXE · 2024-06-06T22:58:38Z

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

kommunarr · 2024-06-06T23:15:33Z

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.

PikachuEXE · 2024-06-06T23:22:54Z

It's less urgent for me as #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)

kommunarr · 2024-06-07T02:43:44Z

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

github-actions · 2024-06-15T12:03:38Z

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

github-actions · 2024-07-16T01:54:50Z

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.

PikachuEXE · 2024-07-16T02:25:38Z

Will revisit after Vue 3 migration done

PikachuEXE added 2 commits May 21, 2024 09:09

$ Extract add video to playlist button into a component

b6d3295

* Show no of playlist a video already added to

bff595d

FreeTubeBot enabled auto-merge (squash) May 21, 2024 02:09

github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 21, 2024

kommunarr reviewed May 21, 2024

View reviewed changes

absidue reviewed May 21, 2024

View reviewed changes

src/renderer/components/ft-playlist-add-video-button/ft-playlist-add-video-button.scss Outdated Show resolved Hide resolved

$ Remove meaningless CSS file

78d10e9

* Update style & button title

d0831f3

PikachuEXE added 2 commits May 23, 2024 10:30

! Fix property type for can be missing view count

c164bea

* Address RTL and style issue

99b5b41

PikachuEXE added 3 commits May 23, 2024 15:09

* Make counter appear top left (for LTR locales)

78ed082

* Hide counter in single user playlist view

57a0bdc

! Fix counter style

d61af90

kommunarr reviewed May 23, 2024

View reviewed changes

src/renderer/components/watch-video-info/watch-video-info.js Outdated Show resolved Hide resolved

kommunarr added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 23, 2024

$ Remove unused import

de255e5

github-actions bot added the PR: merge conflicts / rebase needed label May 25, 2024

absidue reviewed May 25, 2024

View reviewed changes

PikachuEXE added 2 commits May 26, 2024 08:32

$ Remove unused watcher & data

adb2ea3

* Read reactive property less frequently

e38bc8d

github-actions bot removed the PR: merge conflicts / rebase needed label May 26, 2024

PikachuEXE added 2 commits May 26, 2024 08:39

$ Use already translated text

60869e9

* Update title text to be shorter

7440581

absidue reviewed May 26, 2024

View reviewed changes

src/renderer/components/ft-playlist-add-video-button/ft-playlist-add-video-button.js Show resolved Hide resolved

* Pass computed value instead of re-computing

3a4d0a6

PikachuEXE requested a review from absidue May 27, 2024 01:43

PikachuEXE added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels May 27, 2024

efb4f5ff-1298-471a-8973-3d47447115dc reviewed May 29, 2024

View reviewed changes

efb4f5ff-1298-471a-8973-3d47447115dc approved these changes May 30, 2024

View reviewed changes

PikachuEXE requested a review from kommunarr June 6, 2024 22:14

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 Jun 15, 2024

github-actions bot added the PR: stale label Jul 16, 2024

PikachuEXE closed this Jul 16, 2024

auto-merge was automatically disabled July 16, 2024 02:25
Pull request was closed

github-actions bot removed PR: merge conflicts / rebase needed PR: stale labels Jul 16, 2024

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

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

Uh oh!

Conversation

PikachuEXE commented May 21, 2024

Pull Request Type

Related issue

Description

Screenshots

Testing

Desktop

Additional context

Uh oh!

kommunarr May 21, 2024

Choose a reason for hiding this comment

Uh oh!

PikachuEXE May 22, 2024

Choose a reason for hiding this comment

Uh oh!

kommunarr May 22, 2024

Choose a reason for hiding this comment

Uh oh!

PikachuEXE May 26, 2024

Choose a reason for hiding this comment

Uh oh!

kommunarr commented May 21, 2024

Uh oh!

Uh oh!

PikachuEXE commented May 22, 2024

Uh oh!

kommunarr commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PikachuEXE commented May 23, 2024

Uh oh!

kommunarr commented May 23, 2024

Uh oh!

PikachuEXE commented May 23, 2024

Uh oh!

kommunarr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PikachuEXE commented May 24, 2024

Uh oh!

github-actions bot commented May 25, 2024

Uh oh!

absidue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 26, 2024

Uh oh!

absidue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efb4f5ff-1298-471a-8973-3d47447115dc left a comment

Choose a reason for hiding this comment

Uh oh!

PikachuEXE commented May 30, 2024

Uh oh!

kommunarr commented Jun 6, 2024

Uh oh!

PikachuEXE commented Jun 6, 2024

Uh oh!

kommunarr commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PikachuEXE commented Jun 6, 2024

Uh oh!

kommunarr commented May 22, 2024 •

edited

Loading

kommunarr left a comment •

edited

Loading

kommunarr commented Jun 6, 2024 •

edited

Loading