-
Notifications
You must be signed in to change notification settings - Fork 891
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
Make adding duplicate disabled by default #5044
Conversation
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. |
Sorry, to clarify, I mean right-justify the controls so they're grouped |
2b282e1
to
f76c50f
Compare
src/renderer/components/ft-playlist-add-video-prompt/ft-playlist-add-video-prompt.css
Outdated
Show resolved
Hide resolved
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.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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)
src/renderer/components/ft-playlist-add-video-prompt/ft-playlist-add-video-prompt.js
Show resolved
Hide resolved
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 |
Preventing duplicates to be added and removing duplicates are different features IMO |
There was a problem hiding this 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?
I prefer the screenshots in your recent comment |
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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* development: Translated using Weblate (Italian) Translated using Weblate (Chinese (Traditional)) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (French) Translated using Weblate (Spanish) Translated using Weblate (Portuguese (Brazil)) Translated using Weblate (Spanish) Make adding duplicate disabled by default (FreeTubeApp#5044) Switch settings sections and form elements to created lifecycle hook (FreeTubeApp#5224) Translated using Weblate (French) Translated using Weblate (Estonian) Bump sass from 1.77.2 to 1.77.4 (FreeTubeApp#5222) Bump lefthook from 1.6.13 to 1.6.15 (FreeTubeApp#5223) Bump the eslint group with 2 updates (FreeTubeApp#5218) Bump electron from 30.0.8 to 30.0.9 (FreeTubeApp#5221) Bump swiper from 11.1.3 to 11.1.4 (FreeTubeApp#5220) Bump stylelint from 16.6.0 to 16.6.1 in the stylelint group (FreeTubeApp#5219) Add missing IPC channel constants (FreeTubeApp#5216)
* feature/subscription-cache: ! Rename subscriptions to subscription cache & fix outdated mutation reference Translated using Weblate (Italian) Translated using Weblate (Chinese (Traditional)) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (French) Translated using Weblate (Spanish) Translated using Weblate (Portuguese (Brazil)) Translated using Weblate (Spanish) Make adding duplicate disabled by default (FreeTubeApp#5044) Switch settings sections and form elements to created lifecycle hook (FreeTubeApp#5224) Translated using Weblate (French) Translated using Weblate (Estonian) Bump sass from 1.77.2 to 1.77.4 (FreeTubeApp#5222) Bump lefthook from 1.6.13 to 1.6.15 (FreeTubeApp#5223) Bump the eslint group with 2 updates (FreeTubeApp#5218) Bump electron from 30.0.8 to 30.0.9 (FreeTubeApp#5221) Bump swiper from 11.1.3 to 11.1.4 (FreeTubeApp#5220) Bump stylelint from 16.6.0 to 16.6.1 in the stylelint group (FreeTubeApp#5219) Add missing IPC channel constants (FreeTubeApp#5216)
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