-
Notifications
You must be signed in to change notification settings - Fork 502
feat: add model download progress dialog #7897
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
Changes from all commits
ef0e795
dab2da4
d7782fc
d79c351
18f71d1
3510135
bdfac29
2179ffb
b597daf
35ab8b5
17bd52b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| <script setup lang="ts"> | ||
| type Severity = 'default' | 'secondary' | 'warn' | 'danger' | 'contrast' | ||
|
|
||
| const { label, severity = 'default' } = defineProps<{ | ||
| label: string | ||
| severity?: Severity | ||
| }>() | ||
|
|
||
| function badgeClasses(sev: Severity): string { | ||
| const baseClasses = | ||
| 'inline-flex h-3.5 items-center justify-center rounded-full px-1 text-xxxs font-semibold uppercase' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. xxxs is maybe an indication we should scale out text sizes
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're all rem relative: https://tailwindcss.com/docs/font-size |
||
|
|
||
| switch (sev) { | ||
| case 'danger': | ||
| return `${baseClasses} bg-destructive-background text-white` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider using a semantic token instead of hardcoded The danger variant uses 🤖 Prompt for AI Agents |
||
| case 'contrast': | ||
| return `${baseClasses} bg-base-foreground text-base-background` | ||
| case 'warn': | ||
| return `${baseClasses} bg-warning-background text-base-background` | ||
| case 'secondary': | ||
| return `${baseClasses} bg-secondary-background text-base-foreground` | ||
| default: | ||
| return `${baseClasses} bg-primary-background text-base-foreground` | ||
| } | ||
| } | ||
|
Comment on lines
9
to
25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Use The component concatenates Tailwind classes with template strings, which bypasses tailwind-merge conflict resolution. Additionally, the function is re-executed on every render. ♻️ Refactor to use cn() and computed <script setup lang="ts">
+import { computed } from 'vue'
+
+import { cn } from '@/utils/tailwindUtil'
+
type Severity = 'default' | 'secondary' | 'warn' | 'danger' | 'contrast'
const { label, severity = 'default' } = defineProps<{
label: string
severity?: Severity
}>()
-function badgeClasses(sev: Severity): string {
+const badgeClasses = computed(() => {
const baseClasses =
- 'inline-flex h-3.5 items-center justify-center rounded-full px-1 text-xxxs font-semibold uppercase'
+ 'inline-flex h-3.5 items-center justify-center rounded-full px-1 text-xxxs font-semibold uppercase'
- switch (sev) {
+ switch (severity) {
case 'danger':
- return `${baseClasses} bg-destructive-background text-white`
+ return cn(baseClasses, 'bg-destructive-background text-white')
case 'contrast':
- return `${baseClasses} bg-base-foreground text-base-background`
+ return cn(baseClasses, 'bg-base-foreground text-base-background')
case 'warn':
- return `${baseClasses} bg-warning-background text-base-background`
+ return cn(baseClasses, 'bg-warning-background text-base-background')
case 'secondary':
- return `${baseClasses} bg-secondary-background text-base-foreground`
+ return cn(baseClasses, 'bg-secondary-background text-base-foreground')
default:
- return `${baseClasses} bg-primary-background text-white`
+ return cn(baseClasses, 'bg-primary-background text-white')
}
-}
+})
</script>
<template>
- <span :class="badgeClasses(severity)">{{ label }}</span>
+ <span :class="badgeClasses">{{ label }}</span>
</template>Benefits:
Based on coding guidelines and learnings from the repository. 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make an issue to follow up with a fix later.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 57 @DrJKL, I've created issue #7910 to track this refactoring. It's assigned to you and includes references to this PR and review comment. 🧠 Learnings used
Comment on lines
+9
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Use the Per coding guidelines, use the ♻️ Proposed refactor using cn() utility+import { cn } from '@/utils/tailwindUtil'
+
type Severity = 'default' | 'secondary' | 'warn' | 'danger' | 'contrast'
const { label, severity = 'default' } = defineProps<{
label: string
severity?: Severity
}>()
function badgeClasses(sev: Severity): string {
const baseClasses =
'inline-flex h-3.5 items-center justify-center rounded-full px-1 text-xxxs font-semibold uppercase'
switch (sev) {
case 'danger':
- return `${baseClasses} bg-destructive-background text-white`
+ return cn(baseClasses, 'bg-destructive-background text-white')
case 'contrast':
- return `${baseClasses} bg-base-foreground text-base-background`
+ return cn(baseClasses, 'bg-base-foreground text-base-background')
case 'warn':
- return `${baseClasses} bg-warning-background text-base-background`
+ return cn(baseClasses, 'bg-warning-background text-base-background')
case 'secondary':
- return `${baseClasses} bg-secondary-background text-base-foreground`
+ return cn(baseClasses, 'bg-secondary-background text-base-foreground')
default:
- return `${baseClasses} bg-primary-background text-base-foreground`
+ return cn(baseClasses, 'bg-primary-background text-base-foreground')
}
}As per coding guidelines for 🤖 Prompt for AI Agents |
||
| </script> | ||
|
|
||
| <template> | ||
| <span :class="badgeClasses(severity)">{{ label }}</span> | ||
| </template> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| <script setup lang="ts"> | ||
| import { computed } from 'vue' | ||
| import { useI18n } from 'vue-i18n' | ||
|
|
||
| import StatusBadge from '@/components/common/StatusBadge.vue' | ||
| import type { AssetDownload } from '@/stores/assetDownloadStore' | ||
| import { cn } from '@/utils/tailwindUtil' | ||
|
|
||
| const { job } = defineProps<{ | ||
| job: AssetDownload | ||
| }>() | ||
|
|
||
| const { t } = useI18n() | ||
|
|
||
| const progressPercent = computed(() => Math.round(job.progress * 100)) | ||
| const isCompleted = computed(() => job.status === 'completed') | ||
| const isFailed = computed(() => job.status === 'failed') | ||
| const isRunning = computed(() => job.status === 'running') | ||
| const isPending = computed(() => job.status === 'created') | ||
| </script> | ||
|
|
||
| <template> | ||
| <div | ||
| :class=" | ||
| cn( | ||
| 'flex items-center justify-between rounded-lg bg-modal-card-background px-4 py-3', | ||
| isCompleted && 'opacity-50' | ||
| ) | ||
| " | ||
| > | ||
| <div class="flex flex-col"> | ||
| <span class="text-sm text-base-foreground">{{ job.assetName }}</span> | ||
| <span v-if="isRunning" class="text-xs text-muted-foreground"> | ||
| {{ progressPercent }}% | ||
| </span> | ||
DrJKL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </div> | ||
|
|
||
| <div class="flex items-center gap-2"> | ||
| <template v-if="isFailed"> | ||
| <i | ||
| class="icon-[lucide--circle-alert] size-4 text-destructive-background" | ||
| /> | ||
| <StatusBadge :label="t('progressToast.failed')" severity="danger" /> | ||
| </template> | ||
|
|
||
| <template v-else-if="isCompleted"> | ||
| <StatusBadge :label="t('progressToast.finished')" severity="contrast" /> | ||
| </template> | ||
|
|
||
| <template v-else-if="isRunning"> | ||
| <i | ||
| class="icon-[lucide--loader-circle] size-4 animate-spin text-primary-background" | ||
| /> | ||
| <span class="text-xs text-primary-background"> | ||
| {{ progressPercent }}% | ||
| </span> | ||
| </template> | ||
|
|
||
| <template v-else-if="isPending"> | ||
| <span class="text-xs text-muted-foreground"> | ||
| {{ t('progressToast.pending') }} | ||
| </span> | ||
| </template> | ||
| </div> | ||
| </div> | ||
| </template> | ||
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.
nit: Potentially this is something that can be enforced via linter rather than taking up valuable agent context window.
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.
It is in the linter, but for some reason the generations tend to favor the inline style which then triggers a lint error
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.
Want me to remove the example lines?