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

Make adding duplicate disabled by default #5044

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Apr 30, 2024

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Closes #4818

Description

Disable adding duplicate videos to playlists by default (cannot select playlists with all to be added videos already present)
Can be enabled via new toggle only visible on prompt when any duplicate detected

Screenshots

Nothing changed for video when absent in all playlists, only screenshots of adding video(s) with some/all already present in playlists

Adding single video
image

Adding multiple videos (duplicate disallowed)
image

Adding multiple videos (duplicate allowed)
image

Testing

(A) Add one video
(A1) Video absent from all playlists
(A2) Video present in some playlists
(B) Add multiple videos
(B1) Videos absent from all playlists
(B2) Some but not all videos present in some playlists
(B3) All videos present in some playlists

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

Style/UI/Text not finalized

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

Yeah not sure about the placement of that. The Add to Playlist prompt is already very overcrowded as is. Also still not sure if we should go this way at all

@PikachuEXE
Copy link
Collaborator Author

The idea is that user need to be able to enable the option before potentially tabbing into the playlists so they can select the potentially disabled playlist(s) instead of finding out playlists disabled and go back to enable it

No idea where to place it best

@jasonhenriquez
Copy link
Collaborator

jasonhenriquez commented May 4, 2024

Could you justify the three controls at the top all next to each other on the right? It's not a perfect solution for now, but we can reassess in the future.

@PikachuEXE
Copy link
Collaborator Author

Here is my imagined flow:

  1. Enter query text
  2. Optionally search for playlists with matching video title instead of just matching playlist names
  3. Optionally realize that it's already added but want to add duplicate anyway
  4. Sort it when playlist cannot be found

Ideally it should be (4) then (3) but it might look strange if there is an extra row just for one toggle on desktop
image

@jasonhenriquez
Copy link
Collaborator

Sorry, to clarify, I mean right-justify the controls so they're grouped

@PikachuEXE
Copy link
Collaborator Author

Like this?
image
image

@PikachuEXE PikachuEXE force-pushed the feature/playlists/no-duplicate-added-by-default branch from 2b282e1 to f76c50f Compare May 5, 2024 00:27
@PikachuEXE PikachuEXE marked this pull request as ready for review May 23, 2024 02:19
@github-actions github-actions bot added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: WIP labels May 23, 2024
@PikachuEXE PikachuEXE added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 23, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 23, 2024 02:19
Comment on lines 31 to 44
class="matchingVideoToggle"
:label="$t('User Playlists.Playlists with Matching Videos')"
:compact="true"
:default-value="doSearchPlaylistsWithMatchingVideos"
@change="doSearchPlaylistsWithMatchingVideos = !doSearchPlaylistsWithMatchingVideos"
/>
<ft-toggle-switch
v-if="anyPlaylistContainsVideosToBeAdded"
class="allowDuplicateToggle"
:label="$t('User Playlists.AddVideoPrompt.Allow Adding Duplicate Video(s)')"
:compact="true"
:default-value="addingDuplicateVideosEnabled"
@change="addingDuplicateVideosEnabled = !addingDuplicateVideosEnabled"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (blocking): Thanks for moving these to the right. Could you also stack these two toggles vertically (to increase alignment and further visually group the controls)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have any existing design for having vertical grouping inside a row unless width is passed a breakpoint

image

This is not like settings which we have so many so that they are aligned horizontally with limited slots per row (well a table)
image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble understanding, are you expressing that it's difficult to implement, or that it shouldn't be implemented for X reason?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also stack these two toggles vertically

I posted screenshot above (not pushed) to see (1) if this is what you want (2) if it is what you want then it shouldn't be like that since it's not a table and there is enough width

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure I understand. You can put the two toggles in a flex container that is inline-end-aligned, and then have that flex container's content be inline-start-aligned. Which is how I believe you have it implemented in that picture, in which case it looks good. It's not to conserve space (if I'm interpreting what you mean by "there is enough width" correctly), but as I said:

to increase alignment and further visually group the controls

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short version: it looks weird
Long version: It's strange to see 2 elements/components aligned vertically in a row which is not a table and IMO is not an existing design in FT (feel free to name other projects with this design or existing places in FT with this design in case I miss). At the same time these 2 components are only common in type (toggle) but not their purpose. One is for searching/filtering and another one is for affecting the outcome of the action. (1st reason is more important than 2nd one)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally disagree, but this is not a blocking concern, so will leave it to others' discretion and not push the matter

>
<ft-playlist-selector
tabindex="0"
:tabindex="playlistDisabled(playlist._id) ? -1 : 0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought (future): I would also like to refactor this to use input type="checkbox" someday, and similar for other form elements, because us imitating accessible functionality with custom controls ends up being error-prone and more work (this is pre-existing to this PR, just seems especially apparent with what we have to do to imitate the powerful disabled attribute)

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

Only thing im missing tbh is a remove duplicates button within a playlist. Make it only appear when we know there are duplicates in the playlists other wise do not show it

@PikachuEXE
Copy link
Collaborator Author

Preventing duplicates to be added and removing duplicates are different features IMO
I will open another PR for that

Copy link
Member

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the left alignment looked better than grouping the settings together like this.

Stacked isn't needed imo but maybe it looks better on the left side idk?

@PikachuEXE
Copy link
Collaborator Author

I agree left align look more natural / consistent with existing design in FT & in other places
Especially the Playlist with Matching Videos is meant to be grouped with search input above

Question is do you want it to look like OP screenshots or
image

Hiding my code here since not committed yet
.optionsRow {
  display: grid;
  grid-template-columns: repeat(auto-fit, minmax(100px, 1fr));
  align-items: center;
}

.tightOptions {
  display: grid;
  grid-template-columns: repeat(auto-fit, minmax(100px, max-content));
  column-gap: 16px;
  align-items: center;
}

@media only screen and (width <= 800px) {
  .optionsRow {
    /* Switch to rows from columns */
    grid-template-columns: auto;
    grid-template-rows: repeat(auto-fit, auto);
    align-items: stretch;
  }

  .tightOptions {
    /* Switch to rows from columns */
    grid-template-columns: auto;
    grid-template-rows: repeat(auto-fit, auto);
    align-items: stretch;
  }
}

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

I prefer the screenshots in your recent comment

@jasonhenriquez
Copy link
Collaborator

I'd prefer if we move the sort box over to the left as well if we do that, because I don't like the cognitive aspect of the UX of requiring the user's eyes to dart to parse the meaning, but again, not a blocking issue, and does not matter much if we're going to pursue more expansive changes to the modal in the future

@PikachuEXE
Copy link
Collaborator Author

Putting sort box in the right is existing design in FT and I consider that as a separate issue which should not be the concern of this PR

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

Putting sort box in the right is existing design in FT and I consider that as a separate issue which should not be the concern of this PR

I agree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: waiting for review For PRs that are complete, tested, and ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants