Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/design-system/src/css/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@
--inverted-background-hover: var(--color-charcoal-600);
--warning-background: var(--color-gold-400);
--warning-background-hover: var(--color-gold-500);
--success-background: var(--color-jade-600);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Verify color choice consistency with theming pattern.

Both light and dark themes map --success-background to the same color value (--color-jade-600). This deviates from the established pattern where light themes typically use lighter shades (e.g., 400) and dark themes use darker shades (e.g., 600-700):

  • --warning-background: gold-400 (light) vs gold-600 (dark)
  • --destructive-background: coral-500 (light) vs coral-700 (dark)
  • --primary-background: azure-400 (light) vs azure-600 (dark)

Consider whether line 248 should use var(--color-jade-400) for consistency, though this may be an intentional design decision if jade-600 provides optimal contrast in both themes.

♻️ Proposed refactor for pattern consistency
-  --success-background: var(--color-jade-600);
+  --success-background: var(--color-jade-400);

Apply this change only to line 248 (light theme) if the lighter shade is preferred for consistency.

Also applies to: 374-374

🤖 Prompt for AI Agents
In @packages/design-system/src/css/style.css at line 248, The light-theme
mapping for the CSS variable --success-background currently uses
var(--color-jade-600), which breaks the established pattern of lighter shades
for light themes; update the light-theme declaration(s) of --success-background
to use var(--color-jade-400) (replace occurrences where --success-background is
set to var(--color-jade-600) in the light-theme block(s)) and keep the
dark-theme mapping at var(--color-jade-600); after change, verify
contrast/accessibility and that any duplicated occurrences (the other occurrence
of --success-background) are updated only for the light-theme variant.

--border-default: var(--color-smoke-600);
--border-subtle: var(--color-smoke-400);
--muted-background: var(--color-smoke-700);
Expand Down Expand Up @@ -372,6 +373,7 @@
--inverted-background-hover: var(--color-smoke-200);
--warning-background: var(--color-gold-600);
--warning-background-hover: var(--color-gold-500);
--success-background: var(--color-jade-600);
--border-default: var(--color-charcoal-200);
--border-subtle: var(--color-charcoal-300);
--muted-background: var(--color-charcoal-100);
Expand Down Expand Up @@ -516,6 +518,7 @@
--color-inverted-background-hover: var(--inverted-background-hover);
--color-warning-background: var(--warning-background);
--color-warning-background-hover: var(--warning-background-hover);
--color-success-background: var(--success-background);
--color-border-default: var(--border-default);
--color-border-subtle: var(--border-subtle);
--color-muted-background: var(--muted-background);
Expand Down
28 changes: 28 additions & 0 deletions src/components/sidebar/ModeToggle.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<script setup lang="ts">
import Button from '@/components/ui/button/Button.vue'
import { t } from '@/i18n'
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
import { useCommandStore } from '@/stores/commandStore'

const canvasStore = useCanvasStore()
</script>
Comment on lines +4 to +8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Move useCommandStore() instantiation to the script section for consistency.

canvasStore is correctly instantiated in the script section, but useCommandStore() is called inline within the template's @click handlers. For consistency and to avoid repeated store lookups in the template, instantiate commandStore alongside canvasStore.

♻️ Suggested refactor
 import Button from '@/components/ui/button/Button.vue'
 import { t } from '@/i18n'
 import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
 import { useCommandStore } from '@/stores/commandStore'

 const canvasStore = useCanvasStore()
+const commandStore = useCommandStore()

Then update the click handlers:

-      @click="useCommandStore().execute('Comfy.ToggleLinear')"
+      @click="commandStore.execute('Comfy.ToggleLinear')"

Also applies to: 15-15, 23-23

🤖 Prompt for AI Agents
In @src/components/sidebar/ModeToggle.vue around lines 4 - 8, Instantiate
commandStore by calling useCommandStore() in the same script section where
canvasStore is created (move the useCommandStore() call out of the template),
e.g. add const commandStore = useCommandStore() next to const canvasStore =
useCanvasStore(), then update the template @click handlers to reference
commandStore instead of calling useCommandStore() inline (ensure any places
mentioned around the ModeToggle component's click handlers use commandStore).

<template>
<div class="p-1 bg-secondary-background rounded-lg w-10">
<Button
size="icon"
:title="t('linearMode.linearMode')"
:variant="canvasStore.linearMode ? 'inverted' : 'secondary'"
@click="useCommandStore().execute('Comfy.ToggleLinear')"
>
<i class="icon-[lucide--panels-top-left]" />
</Button>
Comment on lines +11 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding aria-label for icon-only buttons.

While title provides tooltip text, aria-label is more reliably announced by screen readers for icon-only buttons. Since these buttons have no visible text, explicit aria-labels would improve accessibility.

♿ Add aria-label for accessibility
     <Button
       size="icon"
       :title="t('linearMode.linearMode')"
+      :aria-label="t('linearMode.linearMode')"
       :variant="canvasStore.linearMode ? 'inverted' : 'secondary'"
       @click="useCommandStore().execute('Comfy.ToggleLinear')"
     >
       <i class="icon-[lucide--panels-top-left]" />
     </Button>
     <Button
       size="icon"
       :title="t('linearMode.graphMode')"
+      :aria-label="t('linearMode.graphMode')"
       :variant="canvasStore.linearMode ? 'secondary' : 'inverted'"
       @click="useCommandStore().execute('Comfy.ToggleLinear')"
     >
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Button
size="icon"
:title="t('linearMode.linearMode')"
:variant="canvasStore.linearMode ? 'inverted' : 'secondary'"
@click="useCommandStore().execute('Comfy.ToggleLinear')"
>
<i class="icon-[lucide--panels-top-left]" />
</Button>
<Button
size="icon"
:title="t('linearMode.linearMode')"
:aria-label="t('linearMode.linearMode')"
:variant="canvasStore.linearMode ? 'inverted' : 'secondary'"
@click="useCommandStore().execute('Comfy.ToggleLinear')"
>
<i class="icon-[lucide--panels-top-left]" />
</Button>
🤖 Prompt for AI Agents
In @src/components/sidebar/ModeToggle.vue around lines 11 - 18, The Button is
icon-only and only uses title for accessibility; add an aria-label prop to the
Button (e.g., :aria-label="t('linearMode.linearMode')") so screen readers
announce it reliably; update the Button component instance that toggles linear
mode (the one using canvasStore.linearMode and
@click="useCommandStore().execute('Comfy.ToggleLinear')") to supply the
aria-label alongside the existing title.

<Button
size="icon"
:title="t('linearMode.graphMode')"
:variant="canvasStore.linearMode ? 'secondary' : 'inverted'"
@click="useCommandStore().execute('Comfy.ToggleLinear')"
>
<i class="icon-[comfy--workflow]" />
</Button>
Comment on lines 11 to 26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add accessible labels to icon-only buttons.

Both buttons are icon-only and should include aria-label attributes to provide accessible names for screen readers. While title provides a tooltip, it is not reliably announced by assistive technologies.

♿ Proposed accessibility fix
     <Button
       size="icon"
       :title="t('linearMode.linearMode')"
+      :aria-label="t('linearMode.linearMode')"
       :variant="canvasStore.linearMode ? 'inverted' : 'secondary'"
       @click="useCommandStore().execute('Comfy.ToggleLinear')"
     >
       <i class="icon-[lucide--panels-top-left]" />
     </Button>
     <Button
-      gize="icon"
+      size="icon"
       :title="t('linearMode.graphMode')"
+      :aria-label="t('linearMode.graphMode')"
       :variant="canvasStore.linearMode ? 'secondary' : 'inverted'"
       @click="useCommandStore().execute('Comfy.ToggleLinear')"
     >
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Button
size="icon"
:title="t('linearMode.linearMode')"
:variant="canvasStore.linearMode ? 'inverted' : 'secondary'"
@click="useCommandStore().execute('Comfy.ToggleLinear')"
>
<i class="icon-[lucide--panels-top-left]" />
</Button>
<Button
gize="icon"
:title="t('linearMode.graphMode')"
:variant="canvasStore.linearMode ? 'secondary' : 'inverted'"
@click="useCommandStore().execute('Comfy.ToggleLinear')"
>
<i class="icon-[comfy--workflow]" />
</Button>
<Button
size="icon"
:title="t('linearMode.linearMode')"
:aria-label="t('linearMode.linearMode')"
:variant="canvasStore.linearMode ? 'inverted' : 'secondary'"
@click="useCommandStore().execute('Comfy.ToggleLinear')"
>
<i class="icon-[lucide--panels-top-left]" />
</Button>
<Button
size="icon"
:title="t('linearMode.graphMode')"
:aria-label="t('linearMode.graphMode')"
:variant="canvasStore.linearMode ? 'secondary' : 'inverted'"
@click="useCommandStore().execute('Comfy.ToggleLinear')"
>
<i class="icon-[comfy--workflow]" />
</Button>
🤖 Prompt for AI Agents
In @src/components/sidebar/ModeToggle.vue around lines 11 - 26, The two
icon-only Button components (the first Button and the second Button with the
typoed prop "gize") need accessible names: add aria-label attributes using the
same localized strings as the title (e.g.,
aria-label="t('linearMode.linearMode')" and
aria-label="t('linearMode.graphMode')"), and while editing fix the "gize" typo
to "size" on the second Button; keep the existing :title and :variant logic and
the @click handlers (useCommandStore().execute('Comfy.ToggleLinear')) unchanged.

</div>
</template>
11 changes: 10 additions & 1 deletion src/components/sidebar/SideToolbar.vue
Original file line number Diff line number Diff line change
Expand Up @@ -44,22 +44,25 @@
<SidebarBottomPanelToggleButton :is-small="isSmall" />
<SidebarShortcutsToggleButton :is-small="isSmall" />
<SidebarSettingsButton :is-small="isSmall" />
<ModeToggle v-if="showLinearToggle" />
</div>
</div>
<HelpCenterPopups :is-small="isSmall" />
</nav>
</template>

<script setup lang="ts">
import { useResizeObserver } from '@vueuse/core'
import { useResizeObserver, whenever } from '@vueuse/core'
import { debounce } from 'es-toolkit/compat'
import { computed, nextTick, onBeforeUnmount, onMounted, ref, watch } from 'vue'

import HelpCenterPopups from '@/components/helpcenter/HelpCenterPopups.vue'
import ComfyMenuButton from '@/components/sidebar/ComfyMenuButton.vue'
import ModeToggle from '@/components/sidebar/ModeToggle.vue'
import SidebarBottomPanelToggleButton from '@/components/sidebar/SidebarBottomPanelToggleButton.vue'
import SidebarSettingsButton from '@/components/sidebar/SidebarSettingsButton.vue'
import SidebarShortcutsToggleButton from '@/components/sidebar/SidebarShortcutsToggleButton.vue'
import { useFeatureFlags } from '@/composables/useFeatureFlags'
import { useSettingStore } from '@/platform/settings/settingStore'
import { useTelemetry } from '@/platform/telemetry'
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
Expand All @@ -84,6 +87,12 @@ const sideToolbarRef = ref<HTMLElement>()
const topToolbarRef = ref<HTMLElement>()
const bottomToolbarRef = ref<HTMLElement>()

const showLinearToggle = ref(useFeatureFlags().flags.linearToggleEnabled)
whenever(
() => canvasStore.linearMode,
() => (showLinearToggle.value = true)
)

const isSmall = computed(
() => settingStore.get('Comfy.Sidebar.Size') === 'small'
)
Expand Down
1 change: 1 addition & 0 deletions src/components/sidebar/tabs/WorkflowsSidebarTab.vue
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<template>
<SidebarTabTemplate
:title="$t('sideToolbar.workflows')"
v-bind="$attrs"
class="workflows-sidebar-tab"
>
<template #tool-buttons>
Expand Down
4 changes: 4 additions & 0 deletions src/components/topbar/WorkflowTab.vue
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
>
<i class="pi pi-bars" />
</Button>
<i
v-else-if="workflowOption.workflow.activeState?.extra?.linearMode"
class="icon-[lucide--panels-top-left] bg-primary-background"
/>
Comment on lines +19 to +22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding accessibility attributes to the linear mode icon.

This icon conveys that the workflow is in linear mode, but screen reader users have no way to access this information. Based on learnings, elements without visible labels should have an aria-label.

♿ Suggested accessibility improvement
     <i
       v-else-if="workflowOption.workflow.activeState?.extra?.linearMode"
       class="icon-[lucide--panels-top-left] bg-primary-background"
+      :aria-label="t('sideToolbar.workflowTab.linearMode')"
     />

Add the corresponding i18n entry to src/locales/en/main.json:

{
  "sideToolbar": {
    "workflowTab": {
      "linearMode": "Simple Mode"
    }
  }
}

Alternatively, if this is purely a visual indicator and not essential information, use aria-hidden="true" instead.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<i
v-else-if="workflowOption.workflow.activeState?.extra?.linearMode"
class="icon-[lucide--panels-top-left] bg-primary-background"
/>
<i
v-else-if="workflowOption.workflow.activeState?.extra?.linearMode"
class="icon-[lucide--panels-top-left] bg-primary-background"
:aria-label="t('sideToolbar.workflowTab.linearMode')"
/>
🤖 Prompt for AI Agents
In @src/components/topbar/WorkflowTab.vue around lines 19 - 22, The linear mode
icon <i> rendered when workflowOption.workflow.activeState?.extra?.linearMode is
true lacks accessibility semantics; update the <i> element in WorkflowTab.vue to
either include an aria-label using the i18n key
sideToolbar.workflowTab.linearMode (add the entry "linearMode": "Simple Mode" to
src/locales/en/main.json) so screen readers announce it, or mark the element as
aria-hidden="true" if it is purely decorative. Ensure the chosen approach is
applied to the exact <i> node gated by
workflowOption.workflow.activeState?.extra?.linearMode.

<span class="workflow-label inline-block max-w-[150px] truncate text-sm">
{{ workflowOption.workflow.filename }}
</span>
Expand Down
76 changes: 76 additions & 0 deletions src/components/ui/Popover.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<script setup lang="ts">
import {
PopoverArrow,
PopoverContent,
PopoverPortal,
PopoverRoot,
PopoverTrigger
} from 'reka-ui'

import Button from '@/components/ui/button/Button.vue'
import { cn } from '@/utils/tailwindUtil'

defineOptions({
inheritAttrs: false
})

defineProps<{
entries?: { label: string; action?: () => void; icon?: string }[][]
icon?: string
to?: string | HTMLElement
}>()
</script>

<template>
<PopoverRoot v-slot="{ close }">
<PopoverTrigger as-child>
<slot name="button">
<Button size="icon">
<i :class="icon ?? 'icon-[lucide--ellipsis]'" />
</Button>
</slot>
Comment on lines +27 to +31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add accessible label to default trigger button.

The default trigger button is icon-only and lacks an accessible label. Screen reader users won't know what the button does. Add an aria-label attribute or use the title attribute (though aria-label is preferred for accessibility).

♿ Proposed fix to add aria-label
       <slot name="button">
-        <Button size="icon">
+        <Button size="icon" aria-label="Open menu">
           <i :class="icon ?? 'icon-[lucide--ellipsis]'" />
         </Button>
       </slot>

Alternatively, allow consumers to provide the label via a prop if the popover's purpose varies by context.

🤖 Prompt for AI Agents
In @src/components/ui/Popover.vue around lines 26 - 30, The default trigger
inside the <slot name="button"> renders an icon-only <Button> which lacks an
accessible label; update the Popover component to add an aria-label on that
default <Button> (e.g., aria-label="Open popover") and expose a prop (prop name
like defaultTriggerLabel or triggerAriaLabel) so consumers can override the
label; modify the template where <Button size="icon"> is rendered and wire the
new prop into the aria-label attribute, ensuring the icon class logic (<i
:class="icon ?? 'icon-[lucide--ellipsis]'"/>) remains unchanged.

</PopoverTrigger>
<PopoverPortal :to>
<PopoverContent
side="bottom"
:side-offset="5"
:collision-padding="10"
v-bind="$attrs"
class="rounded-lg p-2 bg-base-background shadow-sm border border-border-subtle will-change-[transform,opacity] data-[state=open]:data-[side=top]:animate-slideDownAndFade data-[state=open]:data-[side=right]:animate-slideLeftAndFade data-[state=open]:data-[side=bottom]:animate-slideUpAndFade data-[state=open]:data-[side=left]:animate-slideRightAndFade"
>
<slot>
<div class="flex flex-col p-1">
<section
v-for="(entryGroup, index) in entries ?? []"
:key="index"
class="flex flex-col border-b-2 last:border-none border-border-subtle"
>
<div
v-for="{ label, action, icon } in entryGroup"
:key="label"
Comment on lines +49 to +50
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use unique keys in v-for loop.

Using label as the key assumes all labels are unique within a group. If duplicate labels exist, Vue's reconciliation may behave unexpectedly. Consider using a combination of index and label, or a unique identifier if available.

🔑 Proposed fix
               <div
-                v-for="{ label, action, icon } in entryGroup"
-                :key="label"
+                v-for="({ label, action, icon }, itemIndex) in entryGroup"
+                :key="`${index}-${itemIndex}`"
                 :class="
🤖 Prompt for AI Agents
In @src/components/ui/Popover.vue around lines 49 - 50, The v-for loop currently
uses label as the key (v-for="{ label, action, icon } in entryGroup"
:key="label"), which can break reconciliation if labels are not unique; change
the key to a unique value such as a combination of label and the loop index or a
dedicated id if available (e.g., use the index from v-for or entry.id) so :key
becomes a stable unique identifier for each entry in entryGroup.

:class="
cn(
'flex flex-row gap-4 p-2 rounded-sm my-1',
action &&
'cursor-pointer hover:bg-secondary-background-hover'
)
"
@click="
() => {
if (!action) return
action()
close()
}
"
Comment on lines +58 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Simplify click handler.

The arrow function wrapper is unnecessary. Simplify by directly checking and calling the action.

♻️ Proposed refactor
-                @click="
-                  () => {
-                    if (!action) return
-                    action()
-                    close()
-                  }
-                "
+                @click="action && (action(), close())"
🤖 Prompt for AI Agents
In @src/components/ui/Popover.vue around lines 58 - 64, The click handler in
Popover.vue unnecessarily wraps the logic in an arrow function; replace it with
a direct inline handler that checks the action reference and, if present,
invokes action and then calls close (i.e., remove the () => { ... } wrapper and
use a concise conditional invocation of action followed by close), targeting the
existing @click handler and the action and close identifiers.

>
<i v-if="icon" :class="icon" />
{{ label }}
</div>
Comment on lines +48 to +68
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Menu items lack accessibility: use <button> or add ARIA roles and keyboard support.

The menu items are <div> elements with @click handlers. Screen readers won't announce them as interactive, and they're not keyboard-navigable (no focus, no Enter/Space activation).

Consider using <button> elements or adding role="menuitem" with tabindex="0" and keyboard event handlers.

♿ Suggested fix for accessibility
-              <div
+              <button
                 v-for="{ label, action, icon } in entryGroup"
                 :key="label"
                 :class="
                   cn(
-                    'flex flex-row gap-4 p-2 rounded-sm my-1',
+                    'flex flex-row gap-4 p-2 rounded-sm my-1 w-full text-left bg-transparent border-none',
                     action &&
                       'cursor-pointer hover:bg-secondary-background-hover'
                   )
                 "
+                :disabled="!action"
                 @click="
                   () => {
                     if (!action) return
                     action()
                     close()
                   }
                 "
               >
                 <i v-if="icon" :class="icon" />
                 {{ label }}
-              </div>
+              </button>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<div
v-for="{ label, action, icon } in entryGroup"
:key="label"
:class="
cn(
'flex flex-row gap-4 p-2 rounded-sm my-1',
action &&
'cursor-pointer hover:bg-secondary-background-hover'
)
"
@click="
() => {
if (!action) return
action()
close()
}
"
>
<i v-if="icon" :class="icon" />
{{ label }}
</div>
<button
v-for="{ label, action, icon } in entryGroup"
:key="label"
:class="
cn(
'flex flex-row gap-4 p-2 rounded-sm my-1 w-full text-left bg-transparent border-none',
action &&
'cursor-pointer hover:bg-secondary-background-hover'
)
"
:disabled="!action"
@click="
() => {
if (!action) return
action()
close()
}
"
>
<i v-if="icon" :class="icon" />
{{ label }}
</button>
🤖 Prompt for AI Agents
In @src/components/ui/Popover.vue around lines 48 - 68, The menu items rendered
from entryGroup are non-semantic <div>s and lack keyboard/focus/ARIA support;
update the element (the block that iterates v-for over entryGroup) to be a
semantic interactive element (preferably a <button>) or, if you must keep a
non-button, add role="menuitem", tabindex="0", and keydown handlers to activate
action() on Enter/Space and call close(); ensure the click handler still guards
if (!action) return and keep rendering of icon and label unchanged (refer to the
variables entryGroup, label, action, icon and the close() call to locate the
code).

</section>
Comment on lines +43 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add ARIA roles for menu semantics.

The popover content renders a menu structure but lacks proper ARIA roles. Add role="menu" to the container and role="menuitem" to actionable items to improve accessibility for screen reader users.

♿ Proposed accessibility enhancement
           <div class="flex flex-col p-1">
             <section
               v-for="(entryGroup, index) in entries ?? []"
               :key="index"
               class="flex flex-col border-b-2 last:border-none border-border-subtle"
+              role="group"
             >
               <div
                 v-for="{ label, action, icon } in entryGroup"
                 :key="label"
+                :role="action ? 'menuitem' : undefined"
+                :tabindex="action ? 0 : undefined"
                 :class="

Additionally, consider adding keyboard navigation (Enter/Space to activate, arrow keys to navigate) for full accessibility compliance.

🤖 Prompt for AI Agents
In @src/components/ui/Popover.vue around lines 43 - 69, The popover is missing
ARIA menu roles and keyboard activation: add role="menu" to the section
container rendered by the v-for (entries) and role="menuitem" on the inner
actionable item divs that use action and close(); also add tabindex="0" to those
actionable divs and wire a keydown handler on the same element to invoke
action() and close() when Enter or Space is pressed (and ignore if no action),
and optionally implement arrow key handlers on the container (or delegate) to
move focus between menuitems for basic keyboard navigation; locate symbols: the
section v-for, the inner div that destructures { label, action, icon }, and the
close() call to apply these changes.

</div>
</slot>
<PopoverArrow class="fill-base-background stroke-border-subtle" />
</PopoverContent>
</PopoverPortal>
</PopoverRoot>
</template>
29 changes: 29 additions & 0 deletions src/components/ui/TypeformPopoverButton.vue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nit and can be addressed later: This is more specific of a component and should probably be in a special subdirectory of /src/components. /ui is more for things that can/should be moved into the Design System eventually.

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<script setup lang="ts">
import { whenever } from '@vueuse/core'
import { useTemplateRef } from 'vue'

import Popover from '@/components/ui/Popover.vue'
import Button from '@/components/ui/button/Button.vue'

defineProps<{
dataTfWidget: string
}>()

const feedbackRef = useTemplateRef('feedbackRef')

whenever(feedbackRef, () => {
const scriptEl = document.createElement('script')
scriptEl.src = '//embed.typeform.com/next/embed.js'
feedbackRef.value?.appendChild(scriptEl)
})
</script>
<template>
<Popover>
<template #button>
<Button variant="inverted" class="rounded-full size-12">
<i class="icon-[lucide--circle-question-mark] size-6" />
</Button>
</template>
<div ref="feedbackRef" data-tf-auto-resize :data-tf-widget />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Bug: data-tf-widget attribute is not bound to the prop value.

:data-tf-widget without an explicit value binds to undefined, not to the dataTfWidget prop. The Typeform widget ID will not be passed correctly.

🐛 Proposed fix
-    <div ref="feedbackRef" data-tf-auto-resize :data-tf-widget />
+    <div ref="feedbackRef" data-tf-auto-resize :data-tf-widget="dataTfWidget" />
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<div ref="feedbackRef" data-tf-auto-resize :data-tf-widget />
<div ref="feedbackRef" data-tf-auto-resize :data-tf-widget="dataTfWidget" />
🤖 Prompt for AI Agents
In @src/components/ui/TypeformPopoverButton.vue at line 27, The template
currently uses a shorthand :data-tf-widget with no value which binds undefined
instead of the prop; change the binding so the attribute receives the component
prop (e.g., bind dataTfWidget) by updating the div with ref "feedbackRef" to use
a proper v-bind expression (bind the data-tf-widget attribute to the
dataTfWidget prop) so the Typeform widget ID is passed through; also verify the
prop name dataTfWidget exists on the component and is spelled consistently.

</Popover>
</template>
59 changes: 59 additions & 0 deletions src/components/ui/ZoomPane.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<script setup lang="ts">
import { computed, ref, useTemplateRef } from 'vue'

const zoomPane = useTemplateRef('zoomPane')

const zoom = ref(1.0)
const panX = ref(0.0)
const panY = ref(0.0)

function handleWheel(e: WheelEvent) {
const zoomPaneEl = zoomPane.value
if (!zoomPaneEl) return

zoom.value -= e.deltaY
const { x, y, width, height } = zoomPaneEl.getBoundingClientRect()
const offsetX = e.clientX - x - width / 2
const offsetY = e.clientY - y - height / 2
const scaler = 1.1 ** (e.deltaY / -30)

panY.value = panY.value * scaler - offsetY * (scaler - 1)
panX.value = panX.value * scaler - offsetX * (scaler - 1)
}

let dragging = false
function handleDown(e: PointerEvent) {
if (e.button !== 0) return

const zoomPaneEl = zoomPane.value
if (!zoomPaneEl) return
zoomPaneEl.parentElement?.focus()

zoomPaneEl.setPointerCapture(e.pointerId)
dragging = true
}
function handleMove(e: PointerEvent) {
if (!dragging) return
panX.value += e.movementX
panY.value += e.movementY
}
Comment on lines +35 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Verify pointer button state during drag.

The handleMove function should verify that the primary button is still pressed. If the user releases the button while the pointer is outside the element, dragging might remain true.

Suggested improvement
 function handleMove(e: PointerEvent) {
-  if (!dragging) return
+  if (!dragging.value || !(e.buttons & 1)) return
   panX.value += e.movementX
   panY.value += e.movementY
 }
🤖 Prompt for AI Agents
In @src/components/ui/ZoomPane.vue around lines 40 - 44, handleMove currently
assumes dragging remains true even if the pointer button was released outside
the element; update handleMove(PointerEvent e) to check the primary button state
(e.buttons & 1 or e.buttons === 1) before applying movement to panX and panY,
and if the primary button is not pressed set dragging = false and return so you
stop panning; reference the existing handleMove function and the dragging, panX,
and panY reactive values when making this change.


const transform = computed(() => {
const scale = 1.1 ** (zoom.value / 30)
const matrix = [scale, 0, 0, scale, panX.value, panY.value]
return `matrix(${matrix.join(',')})`
})
</script>
<template>
<div
ref="zoomPane"
class="contain-size flex place-content-center"
@wheel="handleWheel"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Prevent default wheel behavior to avoid page scrolling.

The wheel event handler should call preventDefault() to prevent the page from scrolling while zooming. Currently, users might experience unwanted page scroll when trying to zoom.

Proposed fix
-    @wheel="handleWheel"
+    @wheel.prevent="handleWheel"

Or add preventDefault in the handler:

 function handleWheel(e: WheelEvent) {
+  e.preventDefault()
   const zoomPaneEl = zoomPane.value
   if (!zoomPaneEl) return
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@wheel="handleWheel"
@wheel.prevent="handleWheel"
🤖 Prompt for AI Agents
In @src/components/ui/ZoomPane.vue at line 56, The wheel event on ZoomPane.vue
should prevent default page scrolling during zoom: update the @wheel binding or
the handler so that the wheel event's default is prevented — either change the
template to use the .prevent modifier on the wheel listener or add
event.preventDefault() at the start of the handleWheel(event) method to stop the
page from scrolling while zooming.

@pointerdown.prevent="handleDown"
@pointermove="handleMove"
@pointerup="dragging = false"
@pointercancel="dragging = false"
>
<slot :style="{ transform }" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Document slot style prop assumption.

The slot receives :style="{ transform }", which assumes the slotted content will apply this style to a single root element. If the slot contains multiple root nodes or doesn't forward the style prop, the transform won't work.

Consider: Wrapping the slot content in a <div :style="{ transform }"> within the ZoomPane template to guarantee the transform is applied, or document this requirement clearly for consumers.

🤖 Prompt for AI Agents
In @src/components/ui/ZoomPane.vue at line 57, The ZoomPane slot currently
passes :style="{ transform }" to slotted content but assumes consumers will
apply it to a single root element; to fix, either wrap the <slot> in a container
element that receives :style="{ transform }" inside the ZoomPane template (so
the transform is always applied) or explicitly document in the ZoomPane
API/readme that consumers must accept and forward the style prop on a single
root element; update the template around the slot and/or the component docs
accordingly and reference the slot usage and the transform variable in
ZoomPane.vue.

</div>
</template>
Comment on lines 1 to 59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Add accessibility support for keyboard navigation.

The ZoomPane currently only supports mouse/pointer interactions. Consider adding keyboard navigation for accessibility:

  • Arrow keys for panning
  • +/- or Ctrl+Wheel for zooming
  • Home or 0 to reset
  • Appropriate ARIA attributes
Suggested keyboard handler
function handleKeydown(e: KeyboardEvent) {
  const step = e.shiftKey ? 50 : 10
  
  switch (e.key) {
    case 'ArrowLeft':
      e.preventDefault()
      panX.value += step
      break
    case 'ArrowRight':
      e.preventDefault()
      panX.value -= step
      break
    case 'ArrowUp':
      e.preventDefault()
      panY.value += step
      break
    case 'ArrowDown':
      e.preventDefault()
      panY.value -= step
      break
    case '+':
    case '=':
      e.preventDefault()
      zoom.value += 5
      break
    case '-':
      e.preventDefault()
      zoom.value -= 5
      break
    case '0':
    case 'Home':
      e.preventDefault()
      zoom.value = 1
      panX.value = 0
      panY.value = 0
      break
  }
}

Add to template:

<div
  ref="zoomPane"
  tabindex="0"
  role="img"
  :aria-label="$t('zoomPane.label')"
  class="contain-size flex justify-center items-center"
  @wheel.prevent="handleWheel"
  @keydown="handleKeydown"
  ...
>
🤖 Prompt for AI Agents
In @src/components/ui/ZoomPane.vue around lines 1 - 63, ZoomPane lacks keyboard
accessibility; add a keydown handler and ARIA/tabindex attributes to enable
keyboard panning, zooming and reset. Implement a handleKeydown(KeyboardEvent)
function that maps ArrowLeft/Right/Up/Down to adjust panX and panY (respecting
shift for larger steps), '+'/'=' and '-' to change zoom, and '0'/'Home' to reset
zoom/pan; ensure it calls e.preventDefault() for handled keys. Attach the
handler to the root template element (the ref zoomPane) via @keydown and add
tabindex="0", a suitable role (e.g., role="img" or region), and an aria-label;
also change @wheel to use .prevent so keyboard focus + Ctrl/Shift interactions
behave consistently. Make sure to reference existing refs/refs names (zoom,
panX, panY, zoomPane) and update class names to include sensible focusable
styles if needed.

Loading
Loading