-
Notifications
You must be signed in to change notification settings - Fork 638
feat: position properties panel opposite to sidebar #7647
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 6 commits
683098e
acb5229
0143b2b
22793fd
5a87e31
aecf8d8
43d8619
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,86 @@ | ||
| import { expect } from '@playwright/test' | ||
|
|
||
| import { comfyPageFixture as test } from '../../fixtures/ComfyPage' | ||
|
|
||
| test.describe('Properties panel position', () => { | ||
| test.beforeEach(async ({ comfyPage }) => { | ||
| // Open a sidebar tab to ensure sidebar is visible | ||
| await comfyPage.menu.nodeLibraryTab.open() | ||
| await comfyPage.actionbar.propertiesButton.click() | ||
| }) | ||
|
|
||
| test('positions on the right when sidebar is on the left', async ({ | ||
| comfyPage | ||
| }) => { | ||
| await comfyPage.setSetting('Comfy.Sidebar.Location', 'left') | ||
| await comfyPage.nextFrame() | ||
|
|
||
| const propertiesPanel = comfyPage.page.getByTestId('properties-panel') | ||
| const sidebar = comfyPage.page.locator('.side-bar-panel').first() | ||
|
|
||
| await expect(propertiesPanel).toBeVisible() | ||
| await expect(sidebar).toBeVisible() | ||
|
|
||
| const propsBoundingBox = await propertiesPanel.boundingBox() | ||
| const sidebarBoundingBox = await sidebar.boundingBox() | ||
|
|
||
| expect(propsBoundingBox).not.toBeNull() | ||
| expect(sidebarBoundingBox).not.toBeNull() | ||
|
|
||
| // Properties panel should be to the right of the sidebar | ||
| expect(propsBoundingBox!.x).toBeGreaterThan( | ||
| sidebarBoundingBox!.x + sidebarBoundingBox!.width | ||
| ) | ||
| }) | ||
|
|
||
| test('positions on the left when sidebar is on the right', async ({ | ||
| comfyPage | ||
| }) => { | ||
| await comfyPage.setSetting('Comfy.Sidebar.Location', 'right') | ||
| await comfyPage.nextFrame() | ||
|
|
||
| const propertiesPanel = comfyPage.page.getByTestId('properties-panel') | ||
| const sidebar = comfyPage.page.locator('.side-bar-panel').first() | ||
|
|
||
| await expect(propertiesPanel).toBeVisible() | ||
| await expect(sidebar).toBeVisible() | ||
|
|
||
| const propsBoundingBox = await propertiesPanel.boundingBox() | ||
| const sidebarBoundingBox = await sidebar.boundingBox() | ||
|
|
||
| expect(propsBoundingBox).not.toBeNull() | ||
| expect(sidebarBoundingBox).not.toBeNull() | ||
|
|
||
| // Properties panel should be to the left of the sidebar | ||
| expect(propsBoundingBox!.x + propsBoundingBox!.width).toBeLessThan( | ||
| sidebarBoundingBox!.x | ||
| ) | ||
| }) | ||
|
|
||
| test('close button icon updates based on sidebar location', async ({ | ||
| comfyPage | ||
| }) => { | ||
| const propertiesPanel = comfyPage.page.getByTestId('properties-panel') | ||
|
|
||
| // When sidebar is on the left, panel is on the right | ||
| await comfyPage.setSetting('Comfy.Sidebar.Location', 'left') | ||
| await comfyPage.nextFrame() | ||
|
|
||
| await expect(propertiesPanel).toBeVisible() | ||
| const closeButtonLeft = propertiesPanel | ||
| .locator('button[aria-pressed]') | ||
| .locator('i') | ||
| await expect(closeButtonLeft).toBeVisible() | ||
| await expect(closeButtonLeft).toHaveClass(/lucide--panel-right/) | ||
|
|
||
| // When sidebar is on the right, panel is on the left | ||
| await comfyPage.setSetting('Comfy.Sidebar.Location', 'right') | ||
| await comfyPage.nextFrame() | ||
|
|
||
| const closeButtonRight = propertiesPanel | ||
| .locator('button[aria-pressed]') | ||
| .locator('i') | ||
| await expect(closeButtonRight).toBeVisible() | ||
| await expect(closeButtonRight).toHaveClass(/lucide--panel-left/) | ||
| }) | ||
|
csongorczezar marked this conversation as resolved.
|
||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,29 +22,38 @@ | |
| state-storage="local" | ||
| @resizestart="onResizestart" | ||
| > | ||
| <!-- First panel: sidebar when left, properties when right --> | ||
| <SplitterPanel | ||
| v-if="sidebarLocation === 'left' && !focusMode" | ||
| v-if=" | ||
| !focusMode && (sidebarLocation === 'left' || rightSidePanelVisible) | ||
| " | ||
| :class=" | ||
| cn( | ||
| 'side-bar-panel bg-comfy-menu-bg pointer-events-auto', | ||
| sidebarPanelVisible && 'min-w-78' | ||
| ) | ||
| sidebarLocation === 'left' | ||
| ? cn( | ||
| 'side-bar-panel bg-comfy-menu-bg pointer-events-auto', | ||
| sidebarPanelVisible && 'min-w-78' | ||
| ) | ||
| : 'bg-comfy-menu-bg pointer-events-auto' | ||
| " | ||
| :min-size="10" | ||
| :min-size="sidebarLocation === 'left' ? 10 : 15" | ||
| :size="20" | ||
| :style="{ | ||
| display: | ||
| sidebarPanelVisible && sidebarLocation === 'left' | ||
| ? 'flex' | ||
| : 'none' | ||
| }" | ||
| :style="firstPanelStyle" | ||
| :role="sidebarLocation === 'left' ? 'complementary' : undefined" | ||
| :aria-label=" | ||
| sidebarLocation === 'left' ? t('sideToolbar.sidebar') : undefined | ||
| " | ||
| > | ||
| <slot | ||
| v-if="sidebarPanelVisible && sidebarLocation === 'left'" | ||
| v-if="sidebarLocation === 'left' && sidebarPanelVisible" | ||
| name="side-bar-panel" | ||
| /> | ||
| <slot | ||
| v-else-if="sidebarLocation === 'right'" | ||
| name="right-side-panel" | ||
| /> | ||
| </SplitterPanel> | ||
|
csongorczezar marked this conversation as resolved.
|
||
|
|
||
| <!-- Main panel (always present) --> | ||
| <SplitterPanel :size="80" class="flex flex-col"> | ||
| <slot name="topmenu" :sidebar-panel-visible /> | ||
|
|
||
|
|
@@ -73,38 +82,33 @@ | |
| </Splitter> | ||
| </SplitterPanel> | ||
|
|
||
| <!-- Last panel: properties when left, sidebar when right --> | ||
| <SplitterPanel | ||
| v-if="sidebarLocation === 'right' && !focusMode" | ||
| v-if=" | ||
| !focusMode && (sidebarLocation === 'right' || rightSidePanelVisible) | ||
| " | ||
| :class=" | ||
| cn( | ||
| 'side-bar-panel pointer-events-auto', | ||
| sidebarPanelVisible && 'min-w-78' | ||
| ) | ||
| sidebarLocation === 'right' | ||
| ? cn( | ||
| 'side-bar-panel bg-comfy-menu-bg pointer-events-auto', | ||
| sidebarPanelVisible && 'min-w-78' | ||
| ) | ||
| : 'bg-comfy-menu-bg pointer-events-auto' | ||
| " | ||
| :min-size="10" | ||
| :min-size="sidebarLocation === 'right' ? 10 : 15" | ||
| :size="20" | ||
| :style="{ | ||
| display: | ||
| sidebarPanelVisible && sidebarLocation === 'right' | ||
| ? 'flex' | ||
| : 'none' | ||
| }" | ||
| :style="lastPanelStyle" | ||
| :role="sidebarLocation === 'right' ? 'complementary' : undefined" | ||
| :aria-label=" | ||
| sidebarLocation === 'right' ? t('sideToolbar.sidebar') : undefined | ||
| " | ||
| > | ||
| <slot v-if="sidebarLocation === 'left'" name="right-side-panel" /> | ||
| <slot | ||
| v-if="sidebarPanelVisible && sidebarLocation === 'right'" | ||
| v-else-if="sidebarLocation === 'right' && sidebarPanelVisible" | ||
| name="side-bar-panel" | ||
| /> | ||
| </SplitterPanel> | ||
|
csongorczezar marked this conversation as resolved.
|
||
|
|
||
| <!-- Right Side Panel - independent of sidebar --> | ||
| <SplitterPanel | ||
| v-if="rightSidePanelVisible && !focusMode" | ||
| class="bg-comfy-menu-bg pointer-events-auto" | ||
| :min-size="15" | ||
| :size="20" | ||
| > | ||
| <slot name="right-side-panel" /> | ||
| </SplitterPanel> | ||
| </Splitter> | ||
| </div> | ||
| </div> | ||
|
|
@@ -118,6 +122,7 @@ import type { SplitterResizeStartEvent } from 'primevue/splitter' | |
| import SplitterPanel from 'primevue/splitterpanel' | ||
| import { computed } from 'vue' | ||
|
|
||
| import { t } from '@/i18n' | ||
|
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. The global
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. Done! |
||
| import { useSettingStore } from '@/platform/settings/settingStore' | ||
| import { useBottomPanelStore } from '@/stores/workspace/bottomPanelStore' | ||
| import { useRightSidePanelStore } from '@/stores/workspace/rightSidePanelStore' | ||
|
|
@@ -159,12 +164,25 @@ function onResizestart({ originalEvent: event }: SplitterResizeStartEvent) { | |
| } | ||
|
|
||
| /* | ||
| * Force refresh the splitter when right panel visibility changes to recalculate the width | ||
| * Force refresh the splitter when right panel visibility or sidebar location changes | ||
| * to recalculate the width and panel order | ||
| */ | ||
| const splitterRefreshKey = computed(() => { | ||
| return rightSidePanelVisible.value | ||
| ? 'main-splitter-with-right-panel' | ||
| : 'main-splitter' | ||
| return `main-splitter${rightSidePanelVisible.value ? '-with-right-panel' : ''}-${sidebarLocation.value}` | ||
| }) | ||
|
|
||
| const firstPanelStyle = computed(() => { | ||
| if (sidebarLocation.value === 'left') { | ||
| return { display: sidebarPanelVisible.value ? 'flex' : 'none' } | ||
| } | ||
| return undefined | ||
|
Comment on lines
+176
to
+179
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. Question: Could this be done with tailwind utilities instead of through the style attribute?
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. Great observation. With this we are removing the computed styles.
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. Reverted back to the original way, because Tailwind was interacting differently with PrimeVue's Splitter component, causing subtle layout shifts that affect the canvas rendering.
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. 😢 |
||
| }) | ||
|
|
||
| const lastPanelStyle = computed(() => { | ||
| if (sidebarLocation.value === 'right') { | ||
| return { display: sidebarPanelVisible.value ? 'flex' : 'none' } | ||
| } | ||
| return undefined | ||
| }) | ||
|
csongorczezar marked this conversation as resolved.
|
||
| </script> | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.