mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 14:30:41 +00:00
fix: address review feedback on TransformPane
- Remove toBeDefined test that only verified mock factory output - Add test for content bounds offset with negative-coordinate nodes - Remove unused expandToIncludePoint method (YAGNI) - Use reactive props destructuring per project convention
This commit is contained in:
@@ -2,6 +2,7 @@ import { fireEvent, render, screen } from '@testing-library/vue'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { computed, nextTick, reactive } from 'vue'
|
||||
|
||||
import type { NodeLayout } from '@/renderer/core/layout/types'
|
||||
import { useTransformState } from '@/renderer/core/layout/transform/useTransformState'
|
||||
import { createMockCanvas } from '@/utils/__tests__/litegraphTestUtils'
|
||||
|
||||
@@ -9,6 +10,14 @@ import TransformPane from '../transform/TransformPane.vue'
|
||||
|
||||
const mockCamera = reactive({ x: 0, y: 0, z: 1 })
|
||||
|
||||
const { mockNodes, mockVersion } = vi.hoisted(() => {
|
||||
const { ref: createRef } = require('vue')
|
||||
return {
|
||||
mockNodes: createRef(new Map()),
|
||||
mockVersion: createRef(0)
|
||||
}
|
||||
})
|
||||
|
||||
vi.mock('@/renderer/core/layout/transform/useTransformState', () => {
|
||||
const syncWithCanvas = vi.fn()
|
||||
return {
|
||||
@@ -28,8 +37,8 @@ vi.mock('@/renderer/core/layout/transform/useTransformState', () => {
|
||||
|
||||
vi.mock('@/renderer/core/layout/store/layoutStore', () => ({
|
||||
layoutStore: {
|
||||
getAllNodes: () => computed(() => new Map()),
|
||||
getVersion: () => computed(() => 0),
|
||||
getAllNodes: () => computed(() => mockNodes.value),
|
||||
getVersion: () => computed(() => mockVersion.value),
|
||||
onChange: vi.fn(() => () => {})
|
||||
}
|
||||
}))
|
||||
@@ -176,19 +185,42 @@ describe('TransformPane', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('transform state integration', () => {
|
||||
it('should provide transform utilities to child components', () => {
|
||||
describe('content bounds offset', () => {
|
||||
it('should adjust transform to compensate for negative-coordinate nodes', async () => {
|
||||
mockCamera.x = 10
|
||||
mockCamera.y = 20
|
||||
mockCamera.z = 1
|
||||
|
||||
const nodeLayout: NodeLayout = {
|
||||
id: '1',
|
||||
position: { x: -500, y: -300 },
|
||||
size: { width: 200, height: 100 },
|
||||
zIndex: 0,
|
||||
visible: true,
|
||||
bounds: { x: -500, y: -300, width: 200, height: 100 }
|
||||
}
|
||||
mockNodes.value = new Map([['1', nodeLayout]])
|
||||
mockVersion.value = 1
|
||||
|
||||
const mockCanvas = createMockLGraphCanvas()
|
||||
render(TransformPane, {
|
||||
props: {
|
||||
canvas: mockCanvas
|
||||
}
|
||||
props: { canvas: mockCanvas }
|
||||
})
|
||||
|
||||
const transformState = useTransformState()
|
||||
expect(transformState.syncWithCanvas).toBeDefined()
|
||||
expect(transformState.canvasToScreen).toBeDefined()
|
||||
expect(transformState.screenToCanvas).toBeDefined()
|
||||
await nextTick()
|
||||
vi.advanceTimersToNextFrame()
|
||||
await nextTick()
|
||||
|
||||
const pane = screen.getByTestId('transform-pane')
|
||||
const style = pane.getAttribute('style') ?? ''
|
||||
|
||||
// Pane should have explicit dimensions (not 100%)
|
||||
expect(style).toMatch(/width:\s*\d+px/)
|
||||
expect(style).toMatch(/height:\s*\d+px/)
|
||||
|
||||
// Camera translation should be adjusted by offset (camera - offset)
|
||||
// so it won't match the raw camera values
|
||||
expect(style).not.toContain('translate3d(10px, 20px, 0)')
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -31,11 +31,11 @@ interface TransformPaneProps {
|
||||
canvas?: LGraphCanvas
|
||||
}
|
||||
|
||||
const props = defineProps<TransformPaneProps>()
|
||||
const { canvas } = defineProps<TransformPaneProps>()
|
||||
|
||||
const { camera, syncWithCanvas } = useTransformState()
|
||||
|
||||
const canvasElement = computed(() => props.canvas?.canvas)
|
||||
const canvasElement = computed(() => canvas?.canvas)
|
||||
const { isTransforming: isInteracting } = useTransformSettling(canvasElement, {
|
||||
settleDelay: 256
|
||||
})
|
||||
@@ -118,8 +118,8 @@ watch(isInteracting, (interacting) => {
|
||||
|
||||
useRafFn(
|
||||
() => {
|
||||
if (!props.canvas) return
|
||||
syncWithCanvas(props.canvas)
|
||||
if (!canvas) return
|
||||
syncWithCanvas(canvas)
|
||||
updateContentBounds()
|
||||
contentBounds.flush()
|
||||
applyStyles()
|
||||
|
||||
@@ -84,20 +84,6 @@ describe('useContentBounds', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('expandToIncludePoint', () => {
|
||||
it('expands to include a single point', () => {
|
||||
const cb = useContentBounds()
|
||||
cb.expandToIncludePoint({ x: -800, y: 500 })
|
||||
cb.flush()
|
||||
|
||||
expect(cb.offset.x).toBeGreaterThan(800)
|
||||
// y=500 is positive, doesn't push minY below 0 → offset.y stays 0
|
||||
// maxY expands to 500 + margin
|
||||
expect(cb.offset.y).toBe(0)
|
||||
expect(cb.size.height).toBeGreaterThan(500)
|
||||
})
|
||||
})
|
||||
|
||||
describe('flush', () => {
|
||||
it('returns false when nothing changed', () => {
|
||||
const cb = useContentBounds()
|
||||
|
||||
@@ -29,7 +29,6 @@ interface ContentBoundsState {
|
||||
*/
|
||||
export function useContentBounds(): ContentBoundsState & {
|
||||
expandToInclude(bounds: Bounds): void
|
||||
expandToIncludePoint(point: Point): void
|
||||
flush(): boolean
|
||||
reset(): void
|
||||
} {
|
||||
@@ -66,10 +65,6 @@ export function useContentBounds(): ContentBoundsState & {
|
||||
}
|
||||
}
|
||||
|
||||
function expandToIncludePoint(point: Point) {
|
||||
expandToInclude({ x: point.x, y: point.y, width: 0, height: 0 })
|
||||
}
|
||||
|
||||
/**
|
||||
* Applies pending bound changes to the reactive offset and size.
|
||||
* Returns true if the values actually changed.
|
||||
@@ -100,7 +95,6 @@ export function useContentBounds(): ContentBoundsState & {
|
||||
offset: readonly(offset),
|
||||
size: readonly(size),
|
||||
expandToInclude,
|
||||
expandToIncludePoint,
|
||||
flush,
|
||||
reset
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user