-
Notifications
You must be signed in to change notification settings - Fork 621
feat: Node search UX updates #9714
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 25 commits
fd31f9d
0f3b2e0
3cba424
d18243e
08666d8
0772f2a
12fd098
8a30211
320cd82
d30bb01
d49f263
82e6269
6689510
dbb7032
cf4dfce
2f1615c
04c00aa
9100058
bbd1e60
307a1c7
98eac41
bd66617
b6234b9
2a531ff
da2fede
8e5dc15
c46316d
f82f862
92e65aa
cc3d3f1
11b62c4
8f41bc7
c00e285
af77920
5c3de00
3786a46
4f97a90
833a2f5
dce8b87
5796c6d
e405930
e181339
955427b
9363d31
0da4362
702f47f
e8c7fc0
c300f20
225ac3e
63e1faa
76b14dc
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 |
|---|---|---|
| @@ -1,9 +1,17 @@ | ||
| import { clsx } from 'clsx' | ||
| import type { ClassArray } from 'clsx' | ||
| import { twMerge } from 'tailwind-merge' | ||
| import { extendTailwindMerge } from 'tailwind-merge' | ||
|
|
||
| export type { ClassValue } from 'clsx' | ||
|
|
||
| const twMerge = extendTailwindMerge({ | ||
| extend: { | ||
| classGroups: { | ||
| 'font-size': ['text-xxs', 'text-xxxs'] | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| export function cn(...inputs: ClassArray) { | ||
| return twMerge(clsx(inputs)) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| <template> | ||
| <span | ||
| :class=" | ||
| cn( | ||
| 'flex h-5 shrink-0 items-center bg-component-node-widget-background p-1 text-xs', | ||
| rest ? 'rounded-l-full pr-1' : 'rounded-full' | ||
| ) | ||
| " | ||
| > | ||
| <i class="icon-[lucide--component] h-full bg-amber-400" /> | ||
|
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. can we use font size instead of h-full?
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. This was taken directly from |
||
| <span class="truncate" v-text="text" /> | ||
| </span> | ||
| <span | ||
| v-if="rest" | ||
| class="-ml-2.5 max-w-max min-w-0 grow basis-0 truncate rounded-r-full bg-component-node-widget-background" | ||
| > | ||
| <span class="pr-2" v-text="rest" /> | ||
|
pythongosssss marked this conversation as resolved.
|
||
| </span> | ||
| </template> | ||
|
|
||
| <script setup lang="ts"> | ||
| import { cn } from '@/utils/tailwindUtil' | ||
|
|
||
| defineProps<{ | ||
| text: string | ||
| rest?: string | ||
| }>() | ||
| </script> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,14 @@ | ||
| <template> | ||
| <div | ||
| class="flex w-50 flex-col overflow-hidden rounded-2xl border border-border-default bg-base-background" | ||
| class="flex flex-col overflow-hidden rounded-lg border border-border-default bg-base-background" | ||
| :style="{ width: `${BASE_WIDTH_PX * (scaleFactor / BASE_SCALE)}px` }" | ||
| > | ||
| <div ref="previewContainerRef" class="overflow-hidden p-3"> | ||
| <div ref="previewWrapperRef" class="origin-top-left scale-50"> | ||
| <div | ||
| ref="previewWrapperRef" | ||
| class="origin-top-left" | ||
| :style="{ transform: `scale(${scaleFactor})` }" | ||
| > | ||
| <LGraphNodePreview :node-def="nodeDef" position="relative" /> | ||
| </div> | ||
| </div> | ||
|
|
@@ -18,21 +23,21 @@ | |
| <!-- Category Path --> | ||
| <p | ||
| v-if="showCategoryPath && nodeDef.category" | ||
| class="-mt-1 text-xs text-muted-foreground" | ||
| class="-mt-1 truncate text-xs text-muted-foreground" | ||
| > | ||
| {{ nodeDef.category.replaceAll('/', ' > ') }} | ||
| {{ nodeDef.category.replaceAll('/', ' / ') }} | ||
|
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. I think it'd be better to use computed!
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. 👍 |
||
| </p> | ||
|
|
||
| <!-- Badges --> | ||
| <div class="flex flex-wrap gap-2 empty:hidden"> | ||
| <NodePricingBadge :node-def="nodeDef" /> | ||
| <div class="flex flex-wrap gap-2 overflow-hidden empty:hidden"> | ||
| <NodePricingBadge class="max-w-full truncate" :node-def="nodeDef" /> | ||
| <NodeProviderBadge :node-def="nodeDef" /> | ||
| </div> | ||
|
|
||
| <!-- Description --> | ||
| <p | ||
| v-if="nodeDef.description" | ||
| class="m-0 text-[11px] leading-normal font-normal text-muted-foreground" | ||
| class="m-0 max-h-[30vh] overflow-y-auto text-[11px] leading-normal font-normal text-muted-foreground" | ||
|
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. can we use text-xs instead of using text-[11px]?
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'm not aware of the specifics of this, but since this was recently redesigned i'd imagine this was a specific part of the design
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. We’ve defined all text sizes using size tokens, so for things like color and size we typically rely on what’s already defined in Tailwind. Would it be okay to just use text-sm here?
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'd say we should go with what product have said to use, but as stated, I am not aware of the specific for why 11px was chosen and it is a pre-existing styling choice for this unique component. 824f8bb#diff-e708b804b619de1b1aba97d98a71dad92af0d07e7b6b646b8bf5aaf4bb99c013
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. yeah. this is based on the design feedback from julien
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. |
||
| > | ||
| {{ nodeDef.description }} | ||
| </p> | ||
|
|
@@ -99,17 +104,20 @@ import NodeProviderBadge from '@/components/node/NodeProviderBadge.vue' | |
| import LGraphNodePreview from '@/renderer/extensions/vueNodes/components/LGraphNodePreview.vue' | ||
| import type { ComfyNodeDefImpl } from '@/stores/nodeDefStore' | ||
|
|
||
| const SCALE_FACTOR = 0.5 | ||
| const BASE_WIDTH_PX = 200 | ||
| const BASE_SCALE = 0.5 | ||
| const PREVIEW_CONTAINER_PADDING_PX = 24 | ||
|
|
||
| const { | ||
| nodeDef, | ||
| showInputsAndOutputs = true, | ||
| showCategoryPath = false | ||
| showCategoryPath = false, | ||
| scaleFactor = 0.5 | ||
| } = defineProps<{ | ||
| nodeDef: ComfyNodeDefImpl | ||
| showInputsAndOutputs?: boolean | ||
| showCategoryPath?: boolean | ||
| scaleFactor?: number | ||
| }>() | ||
|
|
||
| const previewContainerRef = ref<HTMLElement>() | ||
|
|
@@ -118,7 +126,7 @@ const previewWrapperRef = ref<HTMLElement>() | |
| useResizeObserver(previewWrapperRef, (entries) => { | ||
| const entry = entries[0] | ||
| if (entry && previewContainerRef.value) { | ||
| const scaledHeight = entry.contentRect.height * SCALE_FACTOR | ||
| const scaledHeight = entry.contentRect.height * scaleFactor | ||
| previewContainerRef.value.style.height = `${scaledHeight + PREVIEW_CONTAINER_PADDING_PX}px` | ||
| } | ||
|
pythongosssss marked this conversation as resolved.
|
||
| }) | ||
|
|
||

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.
suggestion: (non-blocking)
\s*between every character means a query like"hi"could match across arbitrary whitespace, e.g."t h i n g". Would\s?(at most one whitespace char) reduce false positives while still supporting the cross-word case?