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.

Sign up for GitHub

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

Jump to bottom

Add feature to set a playlist as quick bookmark target #4518

Merged
merged 12 commits into from
Jan 18, 2024
Merged

Add feature to set a playlist as quick bookmark target #4518

merged 12 commits into from
Jan 18, 2024

Conversation

PikachuEXE
Copy link
Collaborator

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Ability to add video to a playlist via a single click removed in multiple playlist PR 4234

Description

Add back ability to add video to a playlist set by user
Even though it's possible to add multiple entries to a playlist, this feature (the quick bookmark button) can only add one entry to target playlist and remove it (behaves like the old bookmark button)

Feature can be disabled, target playlist can be changed at anytime (the bookmarked indicator would change when target playlist changed)

Screenshots

When it's disabled video list look like as if this feature not added so screenshot not included

image
image
image

Testing

  • Test target playlist can be set/updated to another/unset (i.e. disable the feature)
  • Test videos can be bookmarked/unbookmarked & indicator is display correctly in places
  • Test bookmark indicator is updated when target playlist updated/unset

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

UX/text/icons can be changed here or later

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 4, 2024 00:38
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 4, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

@PikachuEXE did you see #4517?

@kommunarr
Copy link
Collaborator

kommunarr commented Jan 4, 2024 •
edited
Loading

PR differences:

  • horizontal icon alignment here vs vertical on other PR
  • slightly larger gap between icons in other PR
  • remove once here vs remove all dupes on other PR (not too relevant), and as a result, different ways we fix deleteVideoIdByPlaylistId
  • Watch Later kept here vs removed on other PR
  • Favorites playlist is unprotected here vs protected on other PR

Things my PR is missing:

  • Custom bookmarked playlist ability
  • Custom bookmarked playlist button
  • removeVideo fix

Things this PR is missing:

  • Favorites icon in watch-video-info section
  • Minor visual improvement to focus behavior
  • "Unsave" text when video is already favorited
  • Default bookmarked playlist
  • removeVideos fix

Not going to ego it out, can handle this however you prefer @PikachuEXE, as long as all "missing" items are addressed.

@kommunarr kommunarr mentioned this pull request Jan 4, 2024
Closed
1 task
@PikachuEXE
Copy link
Collaborator Author

Addressed

  • Favorites icon in watch-video-info section
  • Minor visual improvement to focus behavior
  • "Unsave" text when video is already favorited (used Remove from instead of Unsave from coz that sounds strange
  • Default bookmarked playlist
  • removeVideos fix

kommunarr
kommunarr reviewed Jan 4, 2024
View reviewed changes
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: Hide bookmark icon for videos in the user playlist page if it is bookmarked.

src/renderer/components/ft-list-video/ft-list-video.js Outdated Show resolved Hide resolved
src/renderer/components/playlist-info/playlist-info.js Outdated Show resolved Hide resolved
src/renderer/components/playlist-info/playlist-info.js Outdated Show resolved Hide resolved
@kommunarr
Copy link
Collaborator

kommunarr commented Jan 4, 2024 •
edited
Loading

Even though it's possible to add multiple entries to a playlist, this feature (the quick bookmark button) can only add one entry to target playlist and remove it (behaves like the old bookmark button)

I don't mind it, but just so you know, un-starring a video removes all of the dupes as well in this PR, same as mine.

@kommunarr
Copy link
Collaborator

Looks like the deleteVideoIdsByPlaylistId function I fixed was not one you used in this PR. Please add the fix here as well, just for the future.

@PikachuEXE
Copy link
Collaborator Author

deleteVideoIdsByPlaylistId fix added
quickBookmarkIconText updated
unused copyToClipboard removed
Replace current quick bookmark target with undo done, different toast text though
Playlist page for current quick bookmark target updated to hide star button

kommunarr
kommunarr previously approved these changes Jan 4, 2024
View reviewed changes
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.

LGTM. Thanks for incorporating feedback in such a short window. I really like the UX now, especially the "Undo" in the toast.

absidue
absidue reviewed Jan 9, 2024
View reviewed changes
src/main/index.js Outdated Show resolved Hide resolved
absidue
absidue previously approved these changes Jan 14, 2024
View reviewed changes
@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 Jan 15, 2024
@github-actions GitHub Actions
Copy link
Contributor

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

@PikachuEXE
Merge branch 'development' into feature/playlist-2023-05-w-quick-book…
8d5f3b2 
…mark

* development: (53 commits)
  Add dearrow support for thumbnails (FreeTubeApp#4520)
  Translated using Weblate (French)
  Fix hardware acceleration flag for Linux (FreeTubeApp#4532)
  Translated using Weblate (Bengali)
  Translated using Weblate (Czech)
  Translated using Weblate (Hungarian)
  Translated using Weblate (Turkish)
  Translated using Weblate (Spanish)
  Translated using Weblate (Arabic)
  Translated using Weblate (Italian)
  Translated using Weblate (Polish)
  Translated using Weblate (Russian)
  Translated using Weblate (French)
  Translated using Weblate (Chinese (Simplified))
  Add toggle to suppress sending additional args to external players (FreeTubeApp#4515)
  Translated using Weblate (English (United Kingdom))
  Translated using Weblate (Italian)
  Translated using Weblate (French)
  Translated using Weblate (Finnish)
  Translated using Weblate (Polish)
  ...

# Conflicts:
#	src/renderer/store/modules/settings.js
@github-actions GitHub Actions
Copy link
Contributor

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

@PikachuEXE
Copy link
Collaborator Author

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

@FreeTubeBot FreeTubeBot merged commit 32a2ad9 into FreeTubeApp:development Jan 18, 2024
5 checks passed
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jan 22, 2024 •
edited
Loading

I dont see the color for the star in the code but in the past there was an issue about the star color not being bright enough. If it is not too much hassle can you maybe make change to the color of the star icon to the brightest color yellow, ffff00.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

Also should we maybe disable this? Users can now set a remote playlist as quick bookmark.

VirtualBoxVM_H69RrFBiKO.mp4

@kommunarr
Copy link
Collaborator

I missed that entirely @efb4f5ff-1298-471a-8973-3d47447115dc - good catch. We need a infoSource === 'user' && appended to the check.

@kommunarr kommunarr mentioned this pull request Jan 22, 2024
Merged
1 task
@PikachuEXE
Copy link
Collaborator Author

@efb4f5ff-1298-471a-8973-3d47447115dc
I have no idea which themes should be updated (for not bright enough)...
image

Is there an existing issue/discussion about it?

@PikachuEXE PikachuEXE mentioned this pull request Jan 23, 2024
Closed
4 tasks
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jan 30, 2024 •
edited
Loading

@efb4f5ff-1298-471a-8973-3d47447115dc I have no idea which themes should be updated (for not bright enough)... image

Is there an existing issue/discussion about it?

Issue was closed in the past by the playlist PR but probably should be reopened #1762

@PikachuEXE PikachuEXE deleted the feature/playlist-2023-05-w-quick-bookmark branch April 2, 2024 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kommunarr kommunarr kommunarr approved these changes

@ChunkyProgrammer ChunkyProgrammer ChunkyProgrammer approved these changes

@absidue absidue absidue approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc Awaiting requested review from efb4f5ff-1298-471a-8973-3d47447115dc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@PikachuEXE @efb4f5ff-1298-471a-8973-3d47447115dc @kommunarr @absidue @ChunkyProgrammer @FreeTubeBot