Make adding duplicate disabled by default #5044
Conversation
PikachuEXE
requested review from
efb4f5ff-1298-471a-8973-3d47447115dc and
jasonhenriquez
PikachuEXE
force-pushed
the
feature/playlists/no-duplicate-added-by-default
branch
from
552b912
to
2b282e1
Compare
|
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 |
|
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 |
|
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. |
|
Here is my imagined flow:
Ideally it should be (4) then (3) but it might look strange if there is an extra row just for one toggle on desktop |
|
Sorry, to clarify, I mean right-justify the controls so they're grouped |
|
Like this? |
PikachuEXE
force-pushed
the
feature/playlists/no-duplicate-added-by-default
branch
from
2b282e1
to
f76c50f
Compare
src/renderer/components/ft-playlist-add-video-prompt/ft-playlist-add-video-prompt.css
Show resolved
Hide resolved
github-actions
bot
added
PR: waiting for review
PikachuEXE
added
the
PR: waiting for review
src/renderer/components/ft-playlist-add-video-prompt/ft-playlist-add-video-prompt.vue
Show resolved
Hide resolved
| 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" | ||
| /> |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
I don't think we have any existing design for having vertical grouping inside a row unless width is passed a breakpoint
This is not like settings which we have so many so that they are aligned horizontally with limited slots per row (well a table)
There was a problem hiding this comment.
I'm having trouble understanding, are you expressing that it's difficult to implement, or that it shouldn't be implemented for X reason?
There was a problem hiding this comment.
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
| > | ||
| <ft-playlist-selector | ||
| tabindex="0" | ||
| :tabindex="playlistDisabled(playlist._id) ? -1 : 0" |
There was a problem hiding this comment.
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)
src/renderer/components/ft-playlist-add-video-prompt/ft-playlist-add-video-prompt.js
Show resolved
Hide resolved
Pull Request Type
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
Adding multiple videos (duplicate disallowed)
Adding multiple videos (duplicate allowed)
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
Additional context
Style/UI/Text not finalized