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

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,20 @@

.optionsRow {
display: grid;
grid-template-columns: repeat(2, 1fr);
grid-template-columns: repeat(3, auto);
column-gap: 32px;
grid-template-rows: 1fr;
align-items: center;
justify-content: end;
kommunarr marked this conversation as resolved.
Show resolved Hide resolved
}

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

Expand All @@ -51,7 +54,11 @@
overflow-y: scroll;
}

.playlist-selector-container {
.playlist-selector-container:not(.disabled) {
/* Make them look selectable */
cursor: pointer;
}

.playlist-selector-container.disabled {
opacity: 0.5;
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export default defineComponent({
updateQueryDebounce: function() {},
lastShownAt: Date.now(),
sortBy: SORT_BY_VALUES.LatestUpdatedFirst,
addingDuplicateVideosEnabled: false,
}
},
computed: {
Expand Down Expand Up @@ -111,6 +112,9 @@ export default defineComponent({
toBeAddedToPlaylistVideoList: function () {
return this.$store.getters.getToBeAddedToPlaylistVideoList
},
toBeAddedToPlaylistVideoIdList: function () {
return this.toBeAddedToPlaylistVideoList.map((v) => v.videoId)
},
newPlaylistDefaultProperties: function () {
return this.$store.getters.getNewPlaylistDefaultProperties
},
Expand Down Expand Up @@ -161,6 +165,23 @@ export default defineComponent({
sortBySelectValues() {
return Object.values(SORT_BY_VALUES)
},

playlistIdsContainingVideosToBeAdded() {
const ids = []

this.allPlaylists.forEach((playlist) => {
const playlistVideoIds = playlist.videos.map((v) => v.videoId)

if (this.toBeAddedToPlaylistVideoIdList.every((vid) => playlistVideoIds.includes(vid))) {
ids.push(playlist._id)
}
})

return ids
},
anyPlaylistContainsVideosToBeAdded() {
return this.playlistIdsContainingVideosToBeAdded.length > 0
},
},
watch: {
allPlaylistsLength(val, oldVal) {
Expand Down Expand Up @@ -202,6 +223,16 @@ export default defineComponent({
// due to enter key press in CreatePlaylistPrompt
nextTick(() => this.$refs.searchBar.focus())
},

addingDuplicateVideosEnabled(val) {
if (val) { return }

// Only care when addingDuplicateVideosEnabled disabled
// Remove disabled playlists
this.selectedPlaylistIdList = this.selectedPlaylistIdList.filter(playlistId => {
return !this.playlistIdsContainingVideosToBeAdded.includes(playlistId)
})
},
},
mounted: function () {
this.updateQueryDebounce = debounce(this.updateQuery, 500)
Expand Down Expand Up @@ -238,10 +269,16 @@ export default defineComponent({
const playlist = this.allPlaylists.find((list) => list._id === selectedPlaylistId)
if (playlist == null) { return }

// Use [].concat to avoid `do not mutate vuex store state outside mutation handlers`
let videosToBeAdded = [].concat(this.toBeAddedToPlaylistVideoList)
if (!this.addingDuplicateVideosEnabled) {
const playlistVideoIds = playlist.videos.map((v) => v.videoId)
videosToBeAdded = videosToBeAdded.filter((v) => !playlistVideoIds.includes(v.videoId))
PikachuEXE marked this conversation as resolved.
Show resolved Hide resolved
}

this.addVideos({
_id: playlist._id,
// Use [].concat to avoid `do not mutate vuex store state outside mutation handlers`
videos: [].concat(this.toBeAddedToPlaylistVideoList),
videos: videosToBeAdded,
})
addedPlaylistIds.add(playlist._id)
// Update playlist's `lastUpdatedAt`
Expand Down Expand Up @@ -281,6 +318,12 @@ export default defineComponent({

getIconForSortPreference: (s) => getIconForSortPreference(s),

playlistDisabled(playlistId) {
if (this.addingDuplicateVideosEnabled) { return false }

return this.playlistIdsContainingVideosToBeAdded.includes(playlistId)
},

...mapActions([
'addVideos',
'updatePlaylist',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,20 @@
class="optionsRow"
>
<ft-toggle-switch
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

@kommunarr kommunarr May 25, 2024

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

@kommunarr kommunarr May 29, 2024

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-select
v-if="allPlaylists.length > 1"
class="sortSelect"
Expand All @@ -48,14 +57,19 @@
<ft-flex-box>
<div
v-for="(playlist, index) in activePlaylists"
:key="playlist._id"
:key="`${playlist._id}-${playlistDisabled(playlist._id)}`"
class="playlist-selector-container"
:class="{
disabled: playlistDisabled(playlist._id),
}"
PikachuEXE marked this conversation as resolved.
Show resolved Hide resolved
>
<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)

:playlist="playlist"
:index="index"
:selected="selectedPlaylistIdList.includes(playlist._id)"
:disabled="playlistDisabled(playlist._id)"
:adding-duplicate-videos-enabled="addingDuplicateVideosEnabled"
@selected="countSelected(playlist._id)"
/>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ export default defineComponent({
type: Boolean,
required: true,
},
disabled: {
type: Boolean,
required: true,
},
addingDuplicateVideosEnabled: {
type: Boolean,
required: true,
},
},
emits: ['selected'],
data: function () {
Expand Down Expand Up @@ -76,10 +84,40 @@ export default defineComponent({
count: this.loneVideoPresenceCountInPlaylist,
})
},
multiVideoPresenceCountInPlaylist() {
if (this.toBeAddedToPlaylistVideoList.length < 2) { return null }

// Count of to be added videos already present in this playlist
const v = this.toBeAddedToPlaylistVideoList.reduce((accumulator, toBeAddedToVideo) => {
return this.playlist.videos.some((pv) => pv.videoId === toBeAddedToVideo.videoId)
? accumulator + 1
: accumulator
}, 0)
// Don't display zero value
return v === 0 ? null : v
},
multiVideoPresenceCountInPlaylistText() {
if (this.multiVideoPresenceCountInPlaylist == null) { return null }

if (this.addingDuplicateVideosEnabled || this.toBeAddedToPlaylistVideoList.length === this.multiVideoPresenceCountInPlaylist) {
return this.$t('User Playlists.AddVideoPrompt.{videoCount}/{totalVideoCount} Videos Already Added', {
videoCount: this.multiVideoPresenceCountInPlaylist,
totalVideoCount: this.toBeAddedToPlaylistVideoList.length,
})
}

return this.$t('User Playlists.AddVideoPrompt.{videoCount}/{totalVideoCount} Videos Will Be Added', {
videoCount: this.toBeAddedToPlaylistVideoList.length - this.multiVideoPresenceCountInPlaylist,
totalVideoCount: this.toBeAddedToPlaylistVideoList.length,
})
},
videoPresenceCountInPlaylistText() {
return this.loneVideoPresenceCountInPlaylistText ?? this.multiVideoPresenceCountInPlaylistText
},
videoPresenceCountInPlaylistTextVisible() {
if (!this.videoPresenceCountInPlaylistTextShouldBeVisible) { return false }

return this.loneVideoPresenceCountInPlaylistText != null
return this.videoPresenceCountInPlaylistText != null
},
},
created: function () {
Expand All @@ -100,6 +138,8 @@ export default defineComponent({
},

toggleSelection: function () {
if (this.disabled) { return }

this.$emit('selected', this.index)
},

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
v-if="videoPresenceCountInPlaylistTextVisible"
class="videoPresenceCount"
>
{{ loneVideoPresenceCountInPlaylistText }}
{{ videoPresenceCountInPlaylistText }}
</div>
</div>
</div>
Expand Down
5 changes: 4 additions & 1 deletion static/locales/en-US.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,12 @@ User Playlists:
Select a playlist to add your N videos to: 'Select a playlist to add your video to | Select a playlist to add your {videoCount} videos to'
N playlists selected: '{playlistCount} Selected'
Search in Playlists: Search in Playlists
Allow Adding Duplicate Video(s): Allow Adding Duplicate Video(s)
Save: Save

Added {count} Times: 'Added {count} Time | Added {count} Times'
Added {count} Times: 'Already Added | Added {count} Times'
"{videoCount}/{totalVideoCount} Videos Will Be Added": '{videoCount}/{totalVideoCount} Videos Will Be Added'
"{videoCount}/{totalVideoCount} Videos Already Added": '{videoCount}/{totalVideoCount} Videos Already Added'

Toast:
You haven't selected any playlist yet.: You haven't selected any playlist yet.
Expand Down
2 changes: 1 addition & 1 deletion static/locales/en_GB.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ User Playlists:
playlists
Select a playlist to add your N videos to: Select a playlist to add your video
to | Select a playlist to add your {videoCount} videos to
Added {count} Times: Added {count} time | Added {count} times
Added {count} Times: Already Added | Added {count} times
CreatePlaylistPrompt:
New Playlist Name: New Playlist name
Create: Create
Expand Down