mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-24 08:44:06 +00:00
[refactor] Unify small modal dialog styles with showSmallLayoutDialog (#8834)
## Summary Extract a shared `showSmallLayoutDialog` utility and move dialog-specific logic into composables, unifying the duplicated `pt` configurations across small modal dialogs. ## Changes - **`showSmallLayoutDialog`**: Added to `dialogService.ts` with a single unified `pt` config for all small modal dialogs (missing nodes, missing models, import failed, node conflict) - **Composables**: Extracted 4 dialog functions from `dialogService` into dedicated composables following the `useSettingsDialog` / `useModelSelectorDialog` pattern: - `useMissingNodesDialog` - `useMissingModelsDialog` - `useImportFailedNodeDialog` - `useNodeConflictDialog` - Each composable uses direct imports, synchronous `show()`, `hide()`, and a `DIALOG_KEY` constant - Updated all call sites (`app.ts`, `useHelpCenter`, `PackEnableToggle`, `PackInstallButton`, `useImportFailedDetection`) ## Review Focus - Unified `pt` config removes minor style variations between dialogs — intentional design unification ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8834-refactor-Unify-small-modal-dialog-styles-with-showSmallLayoutDialog-3056d73d365081b6963beffc0e5943bf) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: github-actions <github-actions@github.com>
This commit is contained in:
@@ -36,11 +36,11 @@ import ToggleSwitch from 'primevue/toggleswitch'
|
||||
import { computed, ref } from 'vue'
|
||||
import { useI18n } from 'vue-i18n'
|
||||
|
||||
import { useDialogService } from '@/services/dialogService'
|
||||
import type { components } from '@/types/comfyRegistryTypes'
|
||||
import { useConflictAcknowledgment } from '@/workbench/extensions/manager/composables/useConflictAcknowledgment'
|
||||
import { useImportFailedDetection } from '@/workbench/extensions/manager/composables/useImportFailedDetection'
|
||||
import { useComfyManagerStore } from '@/workbench/extensions/manager/stores/comfyManagerStore'
|
||||
import { useNodeConflictDialog } from '@/workbench/extensions/manager/composables/useNodeConflictDialog'
|
||||
import { useConflictDetectionStore } from '@/workbench/extensions/manager/stores/conflictDetectionStore'
|
||||
import type { components as ManagerComponents } from '@/workbench/extensions/manager/types/generatedManagerTypes'
|
||||
|
||||
@@ -54,7 +54,7 @@ const { t } = useI18n()
|
||||
const { isPackEnabled, enablePack, disablePack, installedPacks } =
|
||||
useComfyManagerStore()
|
||||
const { getConflictsForPackageByID } = useConflictDetectionStore()
|
||||
const { showNodeConflictDialog } = useDialogService()
|
||||
const { show: showNodeConflictDialog } = useNodeConflictDialog()
|
||||
const { acknowledgmentState, markConflictsAsSeen } = useConflictAcknowledgment()
|
||||
const { showImportFailedDialog } = useImportFailedDetection(nodePack.id || '')
|
||||
|
||||
|
||||
@@ -26,9 +26,9 @@ import { useI18n } from 'vue-i18n'
|
||||
import DotSpinner from '@/components/common/DotSpinner.vue'
|
||||
import Button from '@/components/ui/button/Button.vue'
|
||||
import type { ButtonVariants } from '@/components/ui/button/button.variants'
|
||||
import { useDialogService } from '@/services/dialogService'
|
||||
import type { components } from '@/types/comfyRegistryTypes'
|
||||
import { useConflictDetection } from '@/workbench/extensions/manager/composables/useConflictDetection'
|
||||
import { useNodeConflictDialog } from '@/workbench/extensions/manager/composables/useNodeConflictDialog'
|
||||
import { useComfyManagerStore } from '@/workbench/extensions/manager/stores/comfyManagerStore'
|
||||
import type {
|
||||
ConflictDetail,
|
||||
@@ -55,7 +55,7 @@ const {
|
||||
}>()
|
||||
|
||||
const managerStore = useComfyManagerStore()
|
||||
const { showNodeConflictDialog } = useDialogService()
|
||||
const { show: showNodeConflictDialog } = useNodeConflictDialog()
|
||||
const { t } = useI18n()
|
||||
|
||||
// Check if any of the packs are currently being installed
|
||||
|
||||
@@ -3,15 +3,30 @@ import { setActivePinia } from 'pinia'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { computed, ref } from 'vue'
|
||||
|
||||
import * as dialogService from '@/services/dialogService'
|
||||
import { useImportFailedDetection } from '@/workbench/extensions/manager/composables/useImportFailedDetection'
|
||||
import * as comfyManagerStore from '@/workbench/extensions/manager/stores/comfyManagerStore'
|
||||
import * as conflictDetectionStore from '@/workbench/extensions/manager/stores/conflictDetectionStore'
|
||||
|
||||
// Mock the stores and services
|
||||
vi.mock('@/workbench/extensions/manager/stores/comfyManagerStore')
|
||||
vi.mock('@/workbench/extensions/manager/stores/conflictDetectionStore')
|
||||
vi.mock('@/services/dialogService')
|
||||
const mockIsPackInstalled = vi.fn()
|
||||
const mockGetConflictsForPackageByID = vi.fn()
|
||||
const mockShow = vi.fn()
|
||||
|
||||
vi.mock('@/workbench/extensions/manager/stores/comfyManagerStore', () => ({
|
||||
useComfyManagerStore: () => ({
|
||||
isPackInstalled: mockIsPackInstalled
|
||||
})
|
||||
}))
|
||||
vi.mock('@/workbench/extensions/manager/stores/conflictDetectionStore', () => ({
|
||||
useConflictDetectionStore: () => ({
|
||||
getConflictsForPackageByID: mockGetConflictsForPackageByID
|
||||
})
|
||||
}))
|
||||
vi.mock(
|
||||
'@/workbench/extensions/manager/composables/useImportFailedNodeDialog',
|
||||
() => ({
|
||||
useImportFailedNodeDialog: () => ({
|
||||
show: mockShow
|
||||
})
|
||||
})
|
||||
)
|
||||
vi.mock('vue-i18n', async () => {
|
||||
const actual = await vi.importActual('vue-i18n')
|
||||
return {
|
||||
@@ -23,43 +38,13 @@ vi.mock('vue-i18n', async () => {
|
||||
})
|
||||
|
||||
describe('useImportFailedDetection', () => {
|
||||
let mockComfyManagerStore: ReturnType<
|
||||
typeof comfyManagerStore.useComfyManagerStore
|
||||
>
|
||||
let mockConflictDetectionStore: ReturnType<
|
||||
typeof conflictDetectionStore.useConflictDetectionStore
|
||||
>
|
||||
let mockDialogService: ReturnType<typeof dialogService.useDialogService>
|
||||
|
||||
beforeEach(() => {
|
||||
setActivePinia(createTestingPinia({ stubActions: false }))
|
||||
|
||||
mockComfyManagerStore = {
|
||||
isPackInstalled: vi.fn()
|
||||
} as unknown as ReturnType<typeof comfyManagerStore.useComfyManagerStore>
|
||||
|
||||
mockConflictDetectionStore = {
|
||||
getConflictsForPackageByID: vi.fn()
|
||||
} as unknown as ReturnType<
|
||||
typeof conflictDetectionStore.useConflictDetectionStore
|
||||
>
|
||||
|
||||
mockDialogService = {
|
||||
showErrorDialog: vi.fn(),
|
||||
showImportFailedNodeDialog: vi.fn()
|
||||
} as unknown as ReturnType<typeof dialogService.useDialogService>
|
||||
|
||||
vi.mocked(comfyManagerStore.useComfyManagerStore).mockReturnValue(
|
||||
mockComfyManagerStore
|
||||
)
|
||||
vi.mocked(conflictDetectionStore.useConflictDetectionStore).mockReturnValue(
|
||||
mockConflictDetectionStore
|
||||
)
|
||||
vi.mocked(dialogService.useDialogService).mockReturnValue(mockDialogService)
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
it('should return false for importFailed when package is not installed', () => {
|
||||
vi.mocked(mockComfyManagerStore.isPackInstalled).mockReturnValue(false)
|
||||
mockIsPackInstalled.mockReturnValue(false)
|
||||
|
||||
const { importFailed } = useImportFailedDetection('test-package')
|
||||
|
||||
@@ -67,10 +52,8 @@ describe('useImportFailedDetection', () => {
|
||||
})
|
||||
|
||||
it('should return false for importFailed when no conflicts exist', () => {
|
||||
vi.mocked(mockComfyManagerStore.isPackInstalled).mockReturnValue(true)
|
||||
vi.mocked(
|
||||
mockConflictDetectionStore.getConflictsForPackageByID
|
||||
).mockReturnValue(undefined)
|
||||
mockIsPackInstalled.mockReturnValue(true)
|
||||
mockGetConflictsForPackageByID.mockReturnValue(undefined)
|
||||
|
||||
const { importFailed } = useImportFailedDetection('test-package')
|
||||
|
||||
@@ -78,10 +61,8 @@ describe('useImportFailedDetection', () => {
|
||||
})
|
||||
|
||||
it('should return false for importFailed when conflicts exist but no import_failed type', () => {
|
||||
vi.mocked(mockComfyManagerStore.isPackInstalled).mockReturnValue(true)
|
||||
vi.mocked(
|
||||
mockConflictDetectionStore.getConflictsForPackageByID
|
||||
).mockReturnValue({
|
||||
mockIsPackInstalled.mockReturnValue(true)
|
||||
mockGetConflictsForPackageByID.mockReturnValue({
|
||||
package_id: 'test-package',
|
||||
package_name: 'Test Package',
|
||||
has_conflict: true,
|
||||
@@ -106,10 +87,8 @@ describe('useImportFailedDetection', () => {
|
||||
})
|
||||
|
||||
it('should return true for importFailed when import_failed conflicts exist', () => {
|
||||
vi.mocked(mockComfyManagerStore.isPackInstalled).mockReturnValue(true)
|
||||
vi.mocked(
|
||||
mockConflictDetectionStore.getConflictsForPackageByID
|
||||
).mockReturnValue({
|
||||
mockIsPackInstalled.mockReturnValue(true)
|
||||
mockGetConflictsForPackageByID.mockReturnValue({
|
||||
package_id: 'test-package',
|
||||
package_name: 'Test Package',
|
||||
has_conflict: true,
|
||||
@@ -135,10 +114,8 @@ describe('useImportFailedDetection', () => {
|
||||
|
||||
it('should work with computed ref packageId', () => {
|
||||
const packageId = ref('test-package')
|
||||
vi.mocked(mockComfyManagerStore.isPackInstalled).mockReturnValue(true)
|
||||
vi.mocked(
|
||||
mockConflictDetectionStore.getConflictsForPackageByID
|
||||
).mockReturnValue({
|
||||
mockIsPackInstalled.mockReturnValue(true)
|
||||
mockGetConflictsForPackageByID.mockReturnValue({
|
||||
package_id: 'test-package',
|
||||
package_name: 'Test Package',
|
||||
has_conflict: true,
|
||||
@@ -160,9 +137,7 @@ describe('useImportFailedDetection', () => {
|
||||
|
||||
// Change packageId
|
||||
packageId.value = 'another-package'
|
||||
vi.mocked(
|
||||
mockConflictDetectionStore.getConflictsForPackageByID
|
||||
).mockReturnValue(undefined)
|
||||
mockGetConflictsForPackageByID.mockReturnValue(undefined)
|
||||
|
||||
expect(importFailed.value).toBe(false)
|
||||
})
|
||||
@@ -181,10 +156,8 @@ describe('useImportFailedDetection', () => {
|
||||
}
|
||||
]
|
||||
|
||||
vi.mocked(mockComfyManagerStore.isPackInstalled).mockReturnValue(true)
|
||||
vi.mocked(
|
||||
mockConflictDetectionStore.getConflictsForPackageByID
|
||||
).mockReturnValue({
|
||||
mockIsPackInstalled.mockReturnValue(true)
|
||||
mockGetConflictsForPackageByID.mockReturnValue({
|
||||
package_id: 'test-package',
|
||||
package_name: 'Test Package',
|
||||
has_conflict: true,
|
||||
@@ -213,10 +186,8 @@ describe('useImportFailedDetection', () => {
|
||||
}
|
||||
]
|
||||
|
||||
vi.mocked(mockComfyManagerStore.isPackInstalled).mockReturnValue(true)
|
||||
vi.mocked(
|
||||
mockConflictDetectionStore.getConflictsForPackageByID
|
||||
).mockReturnValue({
|
||||
mockIsPackInstalled.mockReturnValue(true)
|
||||
mockGetConflictsForPackageByID.mockReturnValue({
|
||||
package_id: 'test-package',
|
||||
package_name: 'Test Package',
|
||||
has_conflict: true,
|
||||
@@ -228,7 +199,7 @@ describe('useImportFailedDetection', () => {
|
||||
|
||||
showImportFailedDialog()
|
||||
|
||||
expect(mockDialogService.showImportFailedNodeDialog).toHaveBeenCalledWith({
|
||||
expect(mockShow).toHaveBeenCalledWith({
|
||||
conflictedPackages: expect.arrayContaining([
|
||||
expect.objectContaining({
|
||||
package_id: 'test-package',
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { computed, unref } from 'vue'
|
||||
import type { ComputedRef } from 'vue'
|
||||
|
||||
import { useDialogService } from '@/services/dialogService'
|
||||
import { useImportFailedNodeDialog } from '@/workbench/extensions/manager/composables/useImportFailedNodeDialog'
|
||||
import { useComfyManagerStore } from '@/workbench/extensions/manager/stores/comfyManagerStore'
|
||||
import { useConflictDetectionStore } from '@/workbench/extensions/manager/stores/conflictDetectionStore'
|
||||
import type {
|
||||
@@ -26,14 +26,14 @@ function extractImportFailedConflicts(conflicts?: ConflictDetail[] | null) {
|
||||
* Creating import failed dialog
|
||||
*/
|
||||
function createImportFailedDialog() {
|
||||
const { showImportFailedNodeDialog } = useDialogService()
|
||||
const { show } = useImportFailedNodeDialog()
|
||||
|
||||
return (
|
||||
conflictedPackages: ConflictDetectionResult[] | null,
|
||||
onClose?: () => void
|
||||
) => {
|
||||
if (conflictedPackages && conflictedPackages.length > 0) {
|
||||
void showImportFailedNodeDialog({
|
||||
void show({
|
||||
conflictedPackages,
|
||||
dialogComponentProps: {
|
||||
onClose
|
||||
|
||||
@@ -0,0 +1,43 @@
|
||||
import { useDialogService } from '@/services/dialogService'
|
||||
import type { DialogComponentProps } from '@/stores/dialogStore'
|
||||
import { useDialogStore } from '@/stores/dialogStore'
|
||||
import ImportFailedNodeContent from '@/workbench/extensions/manager/components/manager/ImportFailedNodeContent.vue'
|
||||
import ImportFailedNodeFooter from '@/workbench/extensions/manager/components/manager/ImportFailedNodeFooter.vue'
|
||||
import ImportFailedNodeHeader from '@/workbench/extensions/manager/components/manager/ImportFailedNodeHeader.vue'
|
||||
import type { ConflictDetectionResult } from '@/workbench/extensions/manager/types/conflictDetectionTypes'
|
||||
|
||||
const DIALOG_KEY = 'global-import-failed'
|
||||
|
||||
export function useImportFailedNodeDialog() {
|
||||
const { showSmallLayoutDialog } = useDialogService()
|
||||
const dialogStore = useDialogStore()
|
||||
|
||||
function hide() {
|
||||
dialogStore.closeDialog({ key: DIALOG_KEY })
|
||||
}
|
||||
|
||||
function show(
|
||||
options: {
|
||||
conflictedPackages?: ConflictDetectionResult[]
|
||||
dialogComponentProps?: Omit<DialogComponentProps, 'pt'>
|
||||
} = {}
|
||||
) {
|
||||
const { dialogComponentProps, conflictedPackages = [] } = options
|
||||
|
||||
showSmallLayoutDialog({
|
||||
key: DIALOG_KEY,
|
||||
headerComponent: ImportFailedNodeHeader,
|
||||
footerComponent: ImportFailedNodeFooter,
|
||||
component: ImportFailedNodeContent,
|
||||
dialogComponentProps,
|
||||
props: {
|
||||
conflictedPackages
|
||||
},
|
||||
footerProps: {
|
||||
conflictedPackages
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
return { show, hide }
|
||||
}
|
||||
@@ -0,0 +1,54 @@
|
||||
import { useDialogService } from '@/services/dialogService'
|
||||
import type { DialogComponentProps } from '@/stores/dialogStore'
|
||||
import { useDialogStore } from '@/stores/dialogStore'
|
||||
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 type { ConflictDetectionResult } from '@/workbench/extensions/manager/types/conflictDetectionTypes'
|
||||
|
||||
const DIALOG_KEY = 'global-node-conflict'
|
||||
|
||||
export function useNodeConflictDialog() {
|
||||
const { showSmallLayoutDialog } = useDialogService()
|
||||
const dialogStore = useDialogStore()
|
||||
|
||||
function hide() {
|
||||
dialogStore.closeDialog({ key: DIALOG_KEY })
|
||||
}
|
||||
|
||||
function show(
|
||||
options: {
|
||||
showAfterWhatsNew?: boolean
|
||||
conflictedPackages?: ConflictDetectionResult[]
|
||||
dialogComponentProps?: Omit<DialogComponentProps, 'pt'>
|
||||
buttonText?: string
|
||||
onButtonClick?: () => void
|
||||
} = {}
|
||||
) {
|
||||
const {
|
||||
dialogComponentProps,
|
||||
buttonText,
|
||||
onButtonClick,
|
||||
showAfterWhatsNew,
|
||||
conflictedPackages
|
||||
} = options
|
||||
|
||||
showSmallLayoutDialog({
|
||||
key: DIALOG_KEY,
|
||||
headerComponent: NodeConflictHeader,
|
||||
footerComponent: NodeConflictFooter,
|
||||
component: NodeConflictDialogContent,
|
||||
dialogComponentProps,
|
||||
props: {
|
||||
showAfterWhatsNew,
|
||||
conflictedPackages
|
||||
},
|
||||
footerProps: {
|
||||
buttonText,
|
||||
onButtonClick
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
return { show, hide }
|
||||
}
|
||||
Reference in New Issue
Block a user