mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-22 07:19:41 +00:00
Close zoom menu when toggling minimap visibility (#5974)
## Summary Closes the zoom menu popup when clicking show/hide minimap to prevent the menu from remaining open after toggling. ## Changes - **What**: Adds `close` event emission from `ZoomControlsModal` when minimap toggle is clicked, wired to `hideModal` in parent `GraphCanvasMenu` - **Tests**: Adds unit tests verifying close behavior for minimap toggle vs other commands ## Review Focus This fixes the immediate UX issue where the zoom popup remained open after toggling minimap visibility. However, the minimap toggle's placement within the zoom menu is **not** ideal—it's not intuitive to look for minimap controls within zoom controls. This PR addresses the current UX friction without tackling the broader discoverability issue. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5974-Close-zoom-menu-when-toggling-minimap-visibility-2866d73d365081bdbb0bfeb0da4b8c2b) by [Unito](https://www.unito.io) --------- Co-authored-by: DrJKL <DrJKL0424@gmail.com>
This commit is contained in:
@@ -66,12 +66,22 @@ test.describe('Minimap', () => {
|
||||
await comfyPage.nextFrame()
|
||||
|
||||
await expect(minimapContainer).not.toBeVisible()
|
||||
|
||||
// Open zoom controls dropdown again
|
||||
await zoomControlsButton.click()
|
||||
await comfyPage.nextFrame()
|
||||
|
||||
await expect(toggleButton).toContainText('Show Minimap')
|
||||
|
||||
await toggleButton.click()
|
||||
await comfyPage.nextFrame()
|
||||
|
||||
await expect(minimapContainer).toBeVisible()
|
||||
|
||||
// Open zoom controls dropdown again to verify button text
|
||||
await zoomControlsButton.click()
|
||||
await comfyPage.nextFrame()
|
||||
|
||||
await expect(toggleButton).toContainText('Hide Minimap')
|
||||
})
|
||||
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
<template>
|
||||
<div>
|
||||
<ZoomControlsModal :visible="isModalVisible" />
|
||||
<ZoomControlsModal :visible="isModalVisible" @close="hideModal" />
|
||||
|
||||
<!-- Backdrop -->
|
||||
<div
|
||||
|
||||
@@ -158,6 +158,10 @@ interface Props {
|
||||
|
||||
const props = defineProps<Props>()
|
||||
|
||||
const emit = defineEmits<{
|
||||
close: []
|
||||
}>()
|
||||
|
||||
const interval = ref<number | null>(null)
|
||||
|
||||
// Computed properties for reactive states
|
||||
@@ -177,6 +181,9 @@ const applyZoom = (val: InputNumberInputEvent) => {
|
||||
|
||||
const executeCommand = (command: string) => {
|
||||
void commandStore.execute(command)
|
||||
if (command === 'Comfy.Canvas.ToggleMinimap') {
|
||||
emit('close')
|
||||
}
|
||||
}
|
||||
|
||||
const startRepeat = (command: string) => {
|
||||
|
||||
@@ -1,4 +1,8 @@
|
||||
import { mount } from '@vue/test-utils'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { createI18n } from 'vue-i18n'
|
||||
|
||||
import ZoomControlsModal from '@/components/graph/modals/ZoomControlsModal.vue'
|
||||
|
||||
// Mock functions
|
||||
const mockExecute = vi.fn()
|
||||
@@ -13,7 +17,14 @@ const mockFormatKeySequence = vi.fn().mockReturnValue('Ctrl+')
|
||||
const mockSetAppZoom = vi.fn()
|
||||
const mockSettingGet = vi.fn().mockReturnValue(true)
|
||||
|
||||
const i18n = createI18n({
|
||||
legacy: false,
|
||||
locale: 'en',
|
||||
messages: { en: {} }
|
||||
})
|
||||
|
||||
// Mock dependencies
|
||||
|
||||
vi.mock('@/renderer/extensions/minimap/composables/useMinimap', () => ({
|
||||
useMinimap: () => ({
|
||||
containerStyles: { value: { backgroundColor: '#fff', borderRadius: '8px' } }
|
||||
@@ -41,128 +52,152 @@ vi.mock('@/platform/settings/settingStore', () => ({
|
||||
})
|
||||
}))
|
||||
|
||||
const createWrapper = (props = {}) => {
|
||||
return mount(ZoomControlsModal, {
|
||||
props: {
|
||||
visible: true,
|
||||
...props
|
||||
},
|
||||
global: {
|
||||
plugins: [i18n],
|
||||
stubs: {
|
||||
Button: false,
|
||||
InputNumber: false
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
describe('ZoomControlsModal', () => {
|
||||
it('should have proper props interface', () => {
|
||||
// Test that the component file structure and basic exports work
|
||||
expect(mockExecute).toBeDefined()
|
||||
expect(mockGetCommand).toBeDefined()
|
||||
expect(mockFormatKeySequence).toBeDefined()
|
||||
expect(mockSetAppZoom).toBeDefined()
|
||||
expect(mockSettingGet).toBeDefined()
|
||||
beforeEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
it('should call command store execute when executeCommand is invoked', () => {
|
||||
mockExecute.mockClear()
|
||||
it('should execute zoom in command when zoom in button is clicked', async () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
// Simulate the executeCommand function behavior
|
||||
const executeCommand = (command: string) => {
|
||||
mockExecute(command)
|
||||
}
|
||||
const buttons = wrapper.findAll('button')
|
||||
const zoomInButton = buttons.find((btn) =>
|
||||
btn.text().includes('graphCanvasMenu.zoomIn')
|
||||
)
|
||||
|
||||
expect(zoomInButton).toBeDefined()
|
||||
await zoomInButton!.trigger('mousedown')
|
||||
|
||||
expect(mockExecute).toHaveBeenCalledWith('Comfy.Canvas.ZoomIn')
|
||||
})
|
||||
|
||||
it('should execute zoom out command when zoom out button is clicked', async () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const buttons = wrapper.findAll('button')
|
||||
const zoomOutButton = buttons.find((btn) =>
|
||||
btn.text().includes('graphCanvasMenu.zoomOut')
|
||||
)
|
||||
|
||||
expect(zoomOutButton).toBeDefined()
|
||||
await zoomOutButton!.trigger('mousedown')
|
||||
|
||||
expect(mockExecute).toHaveBeenCalledWith('Comfy.Canvas.ZoomOut')
|
||||
})
|
||||
|
||||
it('should execute fit view command when fit view button is clicked', async () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const buttons = wrapper.findAll('button')
|
||||
const fitViewButton = buttons.find((btn) =>
|
||||
btn.text().includes('zoomControls.zoomToFit')
|
||||
)
|
||||
|
||||
expect(fitViewButton).toBeDefined()
|
||||
await fitViewButton!.trigger('click')
|
||||
|
||||
executeCommand('Comfy.Canvas.FitView')
|
||||
expect(mockExecute).toHaveBeenCalledWith('Comfy.Canvas.FitView')
|
||||
})
|
||||
|
||||
it('should validate zoom input ranges correctly', () => {
|
||||
mockSetAppZoom.mockClear()
|
||||
it('should emit close when minimap toggle button is clicked', async () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
// Simulate the applyZoom function behavior
|
||||
const applyZoom = (val: { value: number }) => {
|
||||
const inputValue = val.value as number
|
||||
if (isNaN(inputValue) || inputValue < 1 || inputValue > 1000) {
|
||||
return
|
||||
}
|
||||
mockSetAppZoom(inputValue)
|
||||
}
|
||||
const minimapButton = wrapper.find('[data-testid="toggle-minimap-button"]')
|
||||
expect(minimapButton.exists()).toBe(true)
|
||||
|
||||
// Test invalid values
|
||||
applyZoom({ value: 0 })
|
||||
applyZoom({ value: 1010 })
|
||||
applyZoom({ value: NaN })
|
||||
await minimapButton.trigger('click')
|
||||
|
||||
expect(mockExecute).toHaveBeenCalledWith('Comfy.Canvas.ToggleMinimap')
|
||||
expect(wrapper.emitted('close')).toBeTruthy()
|
||||
expect(wrapper.emitted('close')).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('should not emit close when other command buttons are clicked', async () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const buttons = wrapper.findAll('button')
|
||||
const fitViewButton = buttons.find((btn) =>
|
||||
btn.text().includes('zoomControls.zoomToFit')
|
||||
)
|
||||
|
||||
expect(fitViewButton).toBeDefined()
|
||||
await fitViewButton!.trigger('click')
|
||||
|
||||
expect(mockExecute).toHaveBeenCalledWith('Comfy.Canvas.FitView')
|
||||
expect(wrapper.emitted('close')).toBeFalsy()
|
||||
})
|
||||
|
||||
it('should call setAppZoomFromPercentage with valid zoom input values', async () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const inputNumber = wrapper.findComponent({ name: 'InputNumber' })
|
||||
expect(inputNumber.exists()).toBe(true)
|
||||
|
||||
// Emit the input event with PrimeVue's InputNumberInputEvent structure
|
||||
await inputNumber.vm.$emit('input', { value: 150 })
|
||||
|
||||
expect(mockSetAppZoom).toHaveBeenCalledWith(150)
|
||||
})
|
||||
|
||||
it('should not call setAppZoomFromPercentage with invalid zoom input values', async () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const inputNumber = wrapper.findComponent({ name: 'InputNumber' })
|
||||
expect(inputNumber.exists()).toBe(true)
|
||||
|
||||
// Test out of range values
|
||||
await inputNumber.vm.$emit('input', { value: 0 })
|
||||
expect(mockSetAppZoom).not.toHaveBeenCalled()
|
||||
|
||||
// Test valid value
|
||||
applyZoom({ value: 50 })
|
||||
expect(mockSetAppZoom).toHaveBeenCalledWith(50)
|
||||
await inputNumber.vm.$emit('input', { value: 1001 })
|
||||
expect(mockSetAppZoom).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('should return correct minimap toggle text based on setting', () => {
|
||||
const t = (key: string) => {
|
||||
const translations: Record<string, string> = {
|
||||
'zoomControls.showMinimap': 'Show Minimap',
|
||||
'zoomControls.hideMinimap': 'Hide Minimap'
|
||||
}
|
||||
return translations[key] || key
|
||||
}
|
||||
|
||||
// Simulate the minimapToggleText computed property
|
||||
const minimapToggleText = () =>
|
||||
mockSettingGet('Comfy.Minimap.Visible')
|
||||
? t('zoomControls.hideMinimap')
|
||||
: t('zoomControls.showMinimap')
|
||||
|
||||
// Test when minimap is visible
|
||||
it('should display "Hide Minimap" when minimap is visible', () => {
|
||||
mockSettingGet.mockReturnValue(true)
|
||||
expect(minimapToggleText()).toBe('Hide Minimap')
|
||||
const wrapper = createWrapper()
|
||||
|
||||
// Test when minimap is hidden
|
||||
const minimapButton = wrapper.find('[data-testid="toggle-minimap-button"]')
|
||||
expect(minimapButton.text()).toContain('zoomControls.hideMinimap')
|
||||
})
|
||||
|
||||
it('should display "Show Minimap" when minimap is hidden', () => {
|
||||
mockSettingGet.mockReturnValue(false)
|
||||
expect(minimapToggleText()).toBe('Show Minimap')
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const minimapButton = wrapper.find('[data-testid="toggle-minimap-button"]')
|
||||
expect(minimapButton.text()).toContain('zoomControls.showMinimap')
|
||||
})
|
||||
|
||||
it('should format keyboard shortcuts correctly', () => {
|
||||
mockFormatKeySequence.mockReturnValue('Ctrl+')
|
||||
it('should display keyboard shortcuts for commands', () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
expect(mockFormatKeySequence()).toBe('Ctrl+')
|
||||
expect(mockGetCommand).toBeDefined()
|
||||
const buttons = wrapper.findAll('button')
|
||||
expect(buttons.length).toBeGreaterThan(0)
|
||||
|
||||
// Each command button should show the keyboard shortcut
|
||||
expect(mockFormatKeySequence).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('should handle repeat command functionality', () => {
|
||||
mockExecute.mockClear()
|
||||
let interval: number | null = null
|
||||
it('should not be visible when visible prop is false', () => {
|
||||
const wrapper = createWrapper({ visible: false })
|
||||
|
||||
// Simulate the repeat functionality
|
||||
const startRepeat = (command: string) => {
|
||||
if (interval) return
|
||||
const cmd = () => mockExecute(command)
|
||||
cmd() // Execute immediately
|
||||
interval = 1 // Mock interval ID
|
||||
}
|
||||
|
||||
const stopRepeat = () => {
|
||||
if (interval) {
|
||||
interval = null
|
||||
}
|
||||
}
|
||||
|
||||
startRepeat('Comfy.Canvas.ZoomIn')
|
||||
expect(mockExecute).toHaveBeenCalledWith('Comfy.Canvas.ZoomIn')
|
||||
|
||||
stopRepeat()
|
||||
expect(interval).toBeNull()
|
||||
})
|
||||
|
||||
it('should have proper filteredMinimapStyles computed property', () => {
|
||||
const mockContainerStyles = {
|
||||
backgroundColor: '#fff',
|
||||
borderRadius: '8px',
|
||||
height: '100px',
|
||||
width: '200px'
|
||||
}
|
||||
|
||||
// Simulate the filteredMinimapStyles computed property
|
||||
const filteredMinimapStyles = () => {
|
||||
return {
|
||||
...mockContainerStyles,
|
||||
height: undefined,
|
||||
width: undefined
|
||||
}
|
||||
}
|
||||
|
||||
const result = filteredMinimapStyles()
|
||||
expect(result.backgroundColor).toBe('#fff')
|
||||
expect(result.borderRadius).toBe('8px')
|
||||
expect(result.height).toBeUndefined()
|
||||
expect(result.width).toBeUndefined()
|
||||
expect(wrapper.find('.absolute').exists()).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user