-
Notifications
You must be signed in to change notification settings - Fork 423
[refactor] Extract dialog composables and add showSmallDialog helper #6846
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
base: main
Are you sure you want to change the base?
Changes from all commits
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,29 @@ | ||
| import MissingNodesContent from '@/components/dialog/content/MissingNodesContent.vue' | ||
| import MissingNodesFooter from '@/components/dialog/content/MissingNodesFooter.vue' | ||
| import MissingNodesHeader from '@/components/dialog/content/MissingNodesHeader.vue' | ||
| import { useDialogService } from '@/services/dialogService' | ||
| import { useDialogStore } from '@/stores/dialogStore' | ||
| import type { ComponentProps } from 'vue-component-type-helpers' | ||
|
|
||
| const DIALOG_KEY = 'global-missing-nodes' | ||
|
|
||
| export const useMissingNodesDialog = () => { | ||
| const dialogService = useDialogService() | ||
| const dialogStore = useDialogStore() | ||
|
|
||
| function hide() { | ||
| dialogStore.closeDialog({ key: DIALOG_KEY }) | ||
| } | ||
|
|
||
| function show(props: ComponentProps<typeof MissingNodesContent>) { | ||
| dialogService.showSmallDialog({ | ||
| key: DIALOG_KEY, | ||
| headerComponent: MissingNodesHeader, | ||
| footerComponent: MissingNodesFooter, | ||
| component: MissingNodesContent, | ||
| props | ||
| }) | ||
| } | ||
|
|
||
| return { show, hide } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import NodeConflictDialogContent from '@/workbench/extensions/manager/components/manager/NodeConflictDialogContent.vue' | ||
| import NodeConflictFooter from '@/workbench/extensions/manager/components/manager/NodeConflictFooter.vue' | ||
| import NodeConflictHeader from '@/workbench/extensions/manager/components/manager/NodeConflictHeader.vue' | ||
| import { useDialogService } from '@/services/dialogService' | ||
| import { useDialogStore } from '@/stores/dialogStore' | ||
| import type { DialogComponentProps } from '@/stores/dialogStore' | ||
| import type { ConflictDetectionResult } from '@/workbench/extensions/manager/types/conflictDetectionTypes' | ||
|
|
||
| const DIALOG_KEY = 'global-node-conflict' | ||
|
|
||
| export const useNodeConflictDialog = () => { | ||
| const dialogService = useDialogService() | ||
| const dialogStore = useDialogStore() | ||
|
|
||
| function hide() { | ||
| dialogStore.closeDialog({ key: DIALOG_KEY }) | ||
| } | ||
|
|
||
| function show( | ||
| options: { | ||
| showAfterWhatsNew?: boolean | ||
| conflictedPackages?: ConflictDetectionResult[] | ||
| dialogComponentProps?: DialogComponentProps | ||
| buttonText?: string | ||
| onButtonClick?: () => void | ||
| } = {} | ||
| ) { | ||
| const { buttonText, onButtonClick, showAfterWhatsNew, conflictedPackages } = | ||
| options | ||
|
|
||
| return dialogService.showSmallDialog({ | ||
| key: DIALOG_KEY, | ||
| headerComponent: NodeConflictHeader, | ||
| footerComponent: NodeConflictFooter, | ||
| component: NodeConflictDialogContent, | ||
| props: { | ||
| showAfterWhatsNew, | ||
| conflictedPackages | ||
| }, | ||
| footerProps: { | ||
| buttonText, | ||
| onButtonClick | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| return { show, hide } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
| <!-- Import Failed List Wrapper --> | ||
| <div | ||
| v-if="importFailedConflicts.length > 0" | ||
| class="flex min-h-8 w-full flex-col rounded-lg bg-base-background" | ||
| class="flex min-h-8 w-full flex-col rounded-lg bg-secondary-background" | ||
|
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: #!/bin/bash
# Read the NodeConflictDialogContent.vue file to understand its structure
cat -n src/workbench/extensions/manager/components/manager/NodeConflictDialogContent.vueRepository: Comfy-Org/ComfyUI_frontend Length of output: 9603 Extension List Wrapper should conditionally render when data is empty, for consistency. The component has three collapsible list sections with identical structure. The "Import Failed List Wrapper" and "Conflict List Wrapper" both use For UI consistency, add 🤖 Prompt for AI Agents |
||
| > | ||
| <div | ||
| data-testid="conflict-dialog-panel-toggle" | ||
|
|
@@ -50,7 +50,7 @@ | |
| <div | ||
| v-for="(packageName, i) in importFailedConflicts" | ||
| :key="i" | ||
| class="conflict-list-item flex h-6 flex-shrink-0 items-center justify-between px-4" | ||
| class="hover:bg-alpha-azure-600-30 flex h-6 flex-shrink-0 items-center justify-between px-4" | ||
| > | ||
| <span class="text-xs text-muted"> | ||
| {{ packageName }} | ||
|
|
@@ -60,7 +60,10 @@ | |
| </div> | ||
| </div> | ||
| <!-- Conflict List Wrapper --> | ||
| <div class="flex min-h-8 w-full flex-col rounded-lg bg-base-background"> | ||
| <div | ||
| v-if="allConflictDetails.length > 0" | ||
| class="flex min-h-8 w-full flex-col rounded-lg bg-secondary-background" | ||
| > | ||
| <div | ||
| data-testid="conflict-dialog-panel-toggle" | ||
| class="flex h-8 w-full items-center justify-between gap-2 pl-4" | ||
|
|
@@ -95,7 +98,7 @@ | |
| <div | ||
| v-for="(conflict, i) in allConflictDetails" | ||
| :key="i" | ||
| class="conflict-list-item flex h-6 flex-shrink-0 items-center justify-between px-4" | ||
| class="hover:bg-alpha-azure-600-30 flex h-6 flex-shrink-0 items-center justify-between px-4" | ||
| > | ||
| <span class="text-xs text-muted">{{ | ||
| getConflictMessage(conflict, t) | ||
|
|
@@ -105,7 +108,9 @@ | |
| </div> | ||
| </div> | ||
| <!-- Extension List Wrapper --> | ||
| <div class="flex min-h-8 w-full flex-col rounded-lg bg-base-background"> | ||
| <div | ||
| class="flex min-h-8 w-full flex-col rounded-lg bg-secondary-background" | ||
| > | ||
| <div | ||
| data-testid="conflict-dialog-panel-toggle" | ||
| class="flex h-8 w-full items-center justify-between gap-2 pl-4" | ||
|
|
@@ -140,7 +145,7 @@ | |
| <div | ||
| v-for="conflictResult in conflictData" | ||
| :key="conflictResult.package_id" | ||
| class="conflict-list-item flex h-6 flex-shrink-0 items-center justify-between px-4" | ||
| class="hover:bg-alpha-azure-600-30 flex h-6 flex-shrink-0 items-center justify-between px-4" | ||
| > | ||
| <span class="text-xs text-muted"> | ||
| {{ conflictResult.package_name }} | ||
|
|
@@ -230,8 +235,3 @@ const toggleExtensionsPanel = () => { | |
| importFailedExpanded.value = false | ||
| } | ||
| </script> | ||
| <style scoped> | ||
| .conflict-list-item:hover { | ||
| background-color: rgb(0 122 255 / 0.2); | ||
| } | ||
| </style> | ||
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 with one concern! Well-structured composable with flexible options.
The composable follows Vue best practices and provides a clean API for displaying node conflict dialogs. However, there's a potential issue:
The
dialogComponentPropsfrom the options parameter (line 23) is destructured (line 28) but not passed toshowSmallDialog()(lines 31-44).Apply this diff to pass through dialog component props:
return dialogService.showSmallDialog({ key: DIALOG_KEY, headerComponent: NodeConflictHeader, footerComponent: NodeConflictFooter, component: NodeConflictDialogContent, props: { showAfterWhatsNew, conflictedPackages }, footerProps: { buttonText, onButtonClick - } + }, + dialogComponentProps })🤖 Prompt for AI Agents