Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions src/locales/en/main.json
Original file line number Diff line number Diff line change
Expand Up @@ -2113,6 +2113,7 @@
"uploadingModel": "Importing model...",
"uploadSuccess": "Model imported successfully!",
"uploadFailed": "Import failed",
"uploadModelHelpVideo": "Upload Model Help Video",
"modelAssociatedWithLink": "The model associated with the link you provided:",
"modelTypeSelectorLabel": "What type of model is this?",
"modelTypeSelectorPlaceholder": "Select model type",
Expand Down
28 changes: 18 additions & 10 deletions src/platform/assets/components/UploadModelFooter.vue
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
<template>
<div class="flex justify-end gap-2 w-full">
<span
<button
v-if="currentStep === 1"
class="text-muted-foreground mr-auto underline flex items-center gap-2"
class="text-muted-foreground mr-auto underline flex items-center gap-2 cursor-pointer bg-transparent border-0 p-0"
data-attr="upload-model-step1-help-link"
@click="showVideoHelp = true"
>
<i class="icon-[lucide--circle-question-mark]" />
<a
href="#"
target="_blank"
class="text-muted-foreground"
data-attr="upload-model-step1-help-link"
>{{ $t('How do I find this?') }}</a
>
</span>
<span>{{ $t('How do I find this?') }}</span>
</button>
<TextButton
v-if="currentStep === 1"
:label="$t('g.cancel')"
Expand Down Expand Up @@ -73,12 +69,24 @@
data-attr="upload-model-step3-finish-button"
@click="emit('close')"
/>
<VideoHelpDialog
v-model="showVideoHelp"
video-url="https://media.comfy.org/compressed_768/civitai_howto.webm"
:aria-label="$t('assetBrowser.uploadModelHelpVideo')"
loop
:show-controls="false"
/>
</div>
</template>

<script setup lang="ts">
import { ref } from 'vue'

import IconTextButton from '@/components/button/IconTextButton.vue'
import TextButton from '@/components/button/TextButton.vue'
import VideoHelpDialog from '@/platform/assets/components/VideoHelpDialog.vue'

const showVideoHelp = ref(false)

defineProps<{
currentStep: number
Expand Down
96 changes: 96 additions & 0 deletions src/platform/assets/components/VideoHelpDialog.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
<template>
<Dialog
v-model:visible="isVisible"
modal
:closable="false"
:close-on-escape="false"
:dismissable-mask="true"
:pt="{
root: { class: 'video-help-dialog' },
header: { class: '!hidden' },
content: { class: '!p-0' },
mask: { class: '!bg-black/70' }
}"
:style="{ width: '90vw', maxWidth: '800px' }"
>
<div class="relative">
<IconButton
class="absolute top-4 right-6 z-10"
:aria-label="$t('g.close')"
@click="isVisible = false"
>
<i class="pi pi-times text-sm" />
</IconButton>
<video
:controls="showControls"
autoplay
muted
:loop="loop"
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional:

Suggested change
:loop="loop"
:loop

:aria-label="ariaLabel"
class="w-full rounded-lg"
:src="videoUrl"
>
{{ $t('g.videoFailedToLoad') }}
</video>
</div>
</Dialog>
</template>
Comment on lines +1 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Component behavior looks solid; consider making the default ariaLabel i18n-aware

Overall the dialog is well-structured: PrimeVue Dialog usage, modelValue/isVisible binding, capture-phase ESC handling with proper add/remove, and video autoplay/accessibility are all implemented correctly.

One minor improvement: the ariaLabel prop default is a hardcoded English string:

withDefaults(defineProps<...>(), {
  ariaLabel: 'Help video',
  loop: true,
  showControls: false
})

Given the i18n guidelines, it would be better to either:

  • Remove the default and force callers to pass a localized label, or
  • Provide a localized fallback, e.g. by falling back in the template:
-      <video
-        :controls="showControls"
-        autoplay
-        muted
-        :loop="loop"
-        :aria-label="ariaLabel"
+      <video
+        :controls="showControls"
+        autoplay
+        muted
+        :loop="loop"
+        :aria-label="ariaLabel || $t('assetBrowser.uploadModelHelpVideo')"
         class="w-full rounded-lg"
         :src="videoUrl"
       >

This ensures that any future use of the component without an explicit aria-label remains localized while keeping the current API.

Also applies to: 45-91

🤖 Prompt for AI Agents
In src/platform/assets/components/VideoHelpDialog.vue around lines 1-37 (and
also apply same change to lines 45-91), the ariaLabel prop currently defaults to
a hardcoded English string which breaks i18n; change the implementation so the
component does not use a hardcoded default but instead uses a localized fallback
at render time (or remove the default prop entirely and require callers to pass
it). Concretely: remove the hardcoded default in withDefaults or make ariaLabel
optional, and in the template compute ariaLabel via the $t() translator when the
prop is absent (e.g. use ariaLabel || $t('...')) so the rendered aria-label is
localized while preserving the existing prop API.


<script setup lang="ts">
import Dialog from 'primevue/dialog'
import { computed, onUnmounted, watch } from 'vue'

import IconButton from '@/components/button/IconButton.vue'

const props = withDefaults(
defineProps<{
modelValue: boolean
videoUrl: string
ariaLabel?: string
loop?: boolean
showControls?: boolean
Comment on lines +47 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

If any of these are always the same, I would remove them as props for now and just set them internally. YAGNI

}>(),
{
ariaLabel: 'Help video',
loop: true,
showControls: false
}
)
Comment on lines +45 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We don't need withDefaults with the version of Vue we're using
https://vuejs.org/guide/components/props.html#reactive-props-destructure
(modelValue intentionally omitted since that can be handled with defineModel)

Suggested change
const props = withDefaults(
defineProps<{
modelValue: boolean
videoUrl: string
ariaLabel?: string
loop?: boolean
showControls?: boolean
}>(),
{
ariaLabel: 'Help video',
loop: true,
showControls: false
}
)
const {
videoUrl,
ariaLabel = 'Help video',
loop = true,
showControls = false
} = defineProps<{
videoUrl: string
ariaLabel?: string
loop?: boolean
showControls?: boolean
}>()


const emit = defineEmits<{
'update:modelValue': [value: boolean]
}>()
Comment on lines +60 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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


const isVisible = computed({
get: () => props.modelValue,
set: (value) => emit('update:modelValue', value)
})

const handleEscapeKey = (event: KeyboardEvent) => {
if (event.key === 'Escape' && isVisible.value) {
event.stopImmediatePropagation()
event.stopPropagation()
event.preventDefault()
isVisible.value = false
}
}

watch(
isVisible,
(visible) => {
if (visible) {
// Add listener with capture phase to intercept before parent dialogs
document.addEventListener('keydown', handleEscapeKey, { capture: true })
} else {
document.removeEventListener('keydown', handleEscapeKey, {
capture: true
})
}
},
{ immediate: true }
)

onUnmounted(() => {
document.removeEventListener('keydown', handleEscapeKey, { capture: true })
})
</script>
Loading