-
Notifications
You must be signed in to change notification settings - Fork 490
[QPOv2] Add media assets viewmode toggle #7729
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 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 | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -63,7 +63,6 @@ import { useWorkflowTemplateSelectorDialog } from './useWorkflowTemplateSelector | |||||||||||||||||||||||||||
| const { isActiveSubscription, showSubscriptionDialog } = useSubscription() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const moveSelectedNodesVersionAdded = '1.22.2' | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| export function useCoreCommands(): ComfyCommand[] { | ||||||||||||||||||||||||||||
| const workflowService = useWorkflowService() | ||||||||||||||||||||||||||||
| const workflowStore = useWorkflowStore() | ||||||||||||||||||||||||||||
|
|
@@ -75,13 +74,22 @@ export function useCoreCommands(): ComfyCommand[] { | |||||||||||||||||||||||||||
| const executionStore = useExecutionStore() | ||||||||||||||||||||||||||||
| const telemetry = useTelemetry() | ||||||||||||||||||||||||||||
| const { staticUrls, buildDocsUrl } = useExternalLink() | ||||||||||||||||||||||||||||
| const settingStore = useSettingStore() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const bottomPanelStore = useBottomPanelStore() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { getSelectedNodes, toggleSelectedNodesMode } = | ||||||||||||||||||||||||||||
| useSelectedLiteGraphItems() | ||||||||||||||||||||||||||||
| const getTracker = () => workflowStore.activeWorkflow?.changeTracker | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| function isQueuePanelV2Enabled() { | ||||||||||||||||||||||||||||
| return settingStore.get('Comfy.Queue.QPOV2') | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| async function toggleQueuePanelV2() { | ||||||||||||||||||||||||||||
| await settingStore.set('Comfy.Queue.QPOV2', !isQueuePanelV2Enabled()) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const moveSelectedNodes = ( | ||||||||||||||||||||||||||||
| positionUpdater: (pos: Point, gridSize: number) => Point | ||||||||||||||||||||||||||||
| ) => { | ||||||||||||||||||||||||||||
|
|
@@ -1175,6 +1183,12 @@ export function useCoreCommands(): ComfyCommand[] { | |||||||||||||||||||||||||||
| await useWorkflowService().reloadCurrentWorkflow() // ensure changes take effect immediately | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| id: 'Comfy.ToggleQPOV2', | ||||||||||||||||||||||||||||
| icon: 'pi pi-list', | ||||||||||||||||||||||||||||
| label: 'Toggle Queue Panel V2', | ||||||||||||||||||||||||||||
| function: toggleQueuePanelV2 | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
|
Comment on lines
+1186
to
+1191
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. 🧹 Nitpick | 🔵 Trivial Consider adding Other commands in this file include a 🔎 Suggested change {
id: 'Comfy.ToggleQPOV2',
icon: 'pi pi-list',
label: 'Toggle Queue Panel V2',
- function: toggleQueuePanelV2
+ function: toggleQueuePanelV2,
+ versionAdded: '1.XX.X'
},📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| id: 'Comfy.ToggleLinear', | ||||||||||||||||||||||||||||
| icon: 'pi pi-database', | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,18 +31,26 @@ | |
| /> | ||
| </template> | ||
| </AssetSortButton> | ||
| <MediaAssetViewModeToggle | ||
| v-if="isQueuePanelV2Enabled" | ||
| v-model:view-mode="viewMode" | ||
| /> | ||
| </div> | ||
| </template> | ||
|
|
||
| <script setup lang="ts"> | ||
| import { computed } from 'vue' | ||
|
|
||
| import SearchBox from '@/components/common/SearchBox.vue' | ||
| import { isCloud } from '@/platform/distribution/types' | ||
| import { useSettingStore } from '@/platform/settings/settingStore' | ||
|
|
||
| import MediaAssetFilterButton from './MediaAssetFilterButton.vue' | ||
| import MediaAssetFilterMenu from './MediaAssetFilterMenu.vue' | ||
| import AssetSortButton from './MediaAssetSortButton.vue' | ||
| import MediaAssetSortMenu from './MediaAssetSortMenu.vue' | ||
| import type { SortBy } from './MediaAssetSortMenu.vue' | ||
| import MediaAssetViewModeToggle from './MediaAssetViewModeToggle.vue' | ||
|
|
||
| const { showGenerationTimeSort = false } = defineProps<{ | ||
| searchQuery: string | ||
|
|
@@ -56,6 +64,12 @@ const emit = defineEmits<{ | |
| }>() | ||
|
|
||
| const sortBy = defineModel<SortBy>('sortBy', { required: true }) | ||
| const viewMode = defineModel<'list' | 'grid'>('viewMode', { required: true }) | ||
|
|
||
| const settingStore = useSettingStore() | ||
| const isQueuePanelV2Enabled = computed(() => | ||
| settingStore.get('Comfy.Queue.QPOV2') | ||
| ) | ||
|
Comment on lines
+67
to
+72
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. 🧹 Nitpick | 🔵 Trivial Consider making The This works correctly but consider if making it optional would reduce unnecessary bindings in parent components when the feature is off. -const viewMode = defineModel<'list' | 'grid'>('viewMode', { required: true })
+const viewMode = defineModel<'list' | 'grid'>('viewMode', { default: 'grid' })Alternatively, the current design is acceptable if you want to ensure consistent state management regardless of feature flag status. 🤖 Prompt for AI Agents |
||
|
|
||
| const handleSearchChange = (value: string | undefined) => { | ||
| emit('update:searchQuery', value ?? '') | ||
|
|
||
|
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. It looks like you have to override a lot of the button styles here.
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.
Member
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. I don't think these are blocking issues so I will merge.
Member
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. @coderabbitai can you add an issue to refactor this for later?
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. 🧩 Analysis chain🏁 Script executed: Length of output: 57 @benceruleanlu, I've created issue #7863 to track the refactoring of 🧠 Learnings used |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,54 @@ | ||||||||||||||||||||
| <template> | ||||||||||||||||||||
| <div | ||||||||||||||||||||
| class="inline-flex items-center gap-1 rounded-lg bg-secondary-background p-1" | ||||||||||||||||||||
| role="group" | ||||||||||||||||||||
| > | ||||||||||||||||||||
|
Comment on lines
+2
to
+5
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. Add accessible label to the toggle group. The 🔎 Suggested fix <div
class="inline-flex items-center gap-1 rounded-lg bg-secondary-background p-1"
role="group"
+ :aria-label="t('sideToolbar.queueProgressOverlay.viewMode')"
>Note: You'll need to add the corresponding key to "viewMode": "View mode"Based on learnings, in this repository, buttons with visible labels don't need aria-label, but this group wrapper needs a label to convey the purpose of the grouped controls to assistive technology users. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
| <Button | ||||||||||||||||||||
| type="button" | ||||||||||||||||||||
| variant="muted-textonly" | ||||||||||||||||||||
| size="icon" | ||||||||||||||||||||
| :aria-label="t('sideToolbar.queueProgressOverlay.viewList')" | ||||||||||||||||||||
| :aria-pressed="viewMode === 'list'" | ||||||||||||||||||||
| :class=" | ||||||||||||||||||||
| cn( | ||||||||||||||||||||
| 'rounded-lg', | ||||||||||||||||||||
| viewMode === 'list' | ||||||||||||||||||||
| ? 'bg-secondary-background-selected text-text-primary hover:bg-secondary-background-selected' | ||||||||||||||||||||
| : 'text-text-secondary hover:bg-secondary-background-hover' | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| " | ||||||||||||||||||||
| @click="viewMode = 'list'" | ||||||||||||||||||||
| > | ||||||||||||||||||||
| <i class="icon-[lucide--table-of-contents] size-4" /> | ||||||||||||||||||||
| </Button> | ||||||||||||||||||||
| <Button | ||||||||||||||||||||
| type="button" | ||||||||||||||||||||
| variant="muted-textonly" | ||||||||||||||||||||
| size="icon" | ||||||||||||||||||||
| :aria-label="t('sideToolbar.queueProgressOverlay.viewGrid')" | ||||||||||||||||||||
| :aria-pressed="viewMode === 'grid'" | ||||||||||||||||||||
| :class=" | ||||||||||||||||||||
| cn( | ||||||||||||||||||||
| 'rounded-lg', | ||||||||||||||||||||
| viewMode === 'grid' | ||||||||||||||||||||
| ? 'bg-secondary-background-selected text-text-primary hover:bg-secondary-background-selected' | ||||||||||||||||||||
| : 'text-text-secondary hover:bg-secondary-background-hover' | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| " | ||||||||||||||||||||
| @click="viewMode = 'grid'" | ||||||||||||||||||||
| > | ||||||||||||||||||||
| <i class="icon-[lucide--layout-grid] size-4" /> | ||||||||||||||||||||
| </Button> | ||||||||||||||||||||
| </div> | ||||||||||||||||||||
| </template> | ||||||||||||||||||||
|
|
||||||||||||||||||||
| <script setup lang="ts"> | ||||||||||||||||||||
| import { useI18n } from 'vue-i18n' | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import Button from '@/components/ui/button/Button.vue' | ||||||||||||||||||||
| import { cn } from '@/utils/tailwindUtil' | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const viewMode = defineModel<'list' | 'grid'>('viewMode', { required: true }) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const { t } = useI18n() | ||||||||||||||||||||
| </script> | ||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1139,5 +1139,13 @@ export const CORE_SETTINGS: SettingParams[] = [ | |||||||||||||||||||||||||||||||||
| type: 'hidden', | ||||||||||||||||||||||||||||||||||
| defaultValue: false, | ||||||||||||||||||||||||||||||||||
| versionAdded: '1.34.1' | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| id: 'Comfy.Queue.QPOV2', | ||||||||||||||||||||||||||||||||||
| name: 'Queue Panel V2', | ||||||||||||||||||||||||||||||||||
| type: 'hidden', | ||||||||||||||||||||||||||||||||||
| tooltip: 'Enable the new Assets Panel design with list/grid view toggle', | ||||||||||||||||||||||||||||||||||
| defaultValue: false, | ||||||||||||||||||||||||||||||||||
| experimental: true | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+1143
to
+1149
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. 🧹 Nitpick | 🔵 Trivial Consider adding Other recently added settings in this file include a 🔎 Suggested change {
id: 'Comfy.Queue.QPOV2',
name: 'Queue Panel V2',
type: 'hidden',
tooltip: 'Enable the new Assets Panel design with list/grid view toggle',
defaultValue: false,
- experimental: true
+ experimental: true,
+ versionAdded: '1.XX.X'
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||
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.
The viewMode state variable is defined but not actually used to control the view display. The VirtualGrid at line 75 always displays in grid mode with a fixed grid layout. Consider either:
This means the toggle button will change the state but won't affect what users see.