fix: UI/UX polish for V2 Node Search (#8987)#9084
Draft
christian-byrne wants to merge 1 commit intomainfrom
Draft
fix: UI/UX polish for V2 Node Search (#8987)#9084christian-byrne wants to merge 1 commit intomainfrom
christian-byrne wants to merge 1 commit intomainfrom
Conversation
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 529 passed, 0 failed · 3 flaky📊 Browser Reports
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds presentational and interaction updates to the v2 node search UI: truncated pricing badge, hover-driven chevron visibility in the category sidebar, restructuring/styling for category tree nodes, per-chip applied-filter counts in the filter bar, and a new button variant. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
christian-byrne
changed the title
📦 Bundle: 4.37 MB gzip 🔴 +526 BDetailsSummary
Category Glance App Entry Points — 17.9 kB (baseline 17.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 918 kB (baseline 916 kB) • 🔴 +2.02 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.8 kB (baseline 68.8 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 436 kB (baseline 436 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 738 B (baseline 738 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 47.1 kB (baseline 47 kB) • 🔴 +158 BReusable component library chunks
Status: 8 added / 8 removed Data & Services — 2.51 MB (baseline 2.51 MB) • 🔴 +60 BStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 58.3 kB (baseline 58.3 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 12 added / 12 removed Vendor & Third-Party — 8.83 MB (baseline 8.83 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.62 MB (baseline 7.62 MB) • ⚪ 0 BBundles that do not match a named category
Status: 53 added / 53 removed |
christian-byrne
force-pushed
the
search-improvements
branch
from
a761311 to
1364170
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/searchbox/v2/NodeSearchFilterBar.vue (1)
3-18: Consider using the shared Button component (optional).The filter chips use raw
<button>elements. The codebase guideline suggests preferring the shared Button component fromsrc/components/ui/button/Button.vue. However, since these chips have specialized three-state visuals (active, has-applied, default) that may not fit existing Button variants, the current approach is acceptable.If this chip pattern is reused elsewhere, consider creating a reusable "chip" variant or a dedicated
FilterChipcomponent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchFilterBar.vue` around lines 3 - 18, The current filter chips render raw <button> elements (see the v-for over chips, chipClass(chip.key), and `@click`="emit('selectChip', chip)") which diverges from the codebase guideline to prefer the shared Button component; either replace each raw button with the shared Button (mapping aria-pressed, class bindings from chipClass, click handler emit('selectChip', chip), and the appliedFilterCounts badge) to keep styling/behavior consistent, or if the three-state visuals are unique, extract this pattern into a reusable FilterChip component (or add a "chip" variant to src/components/ui/button/Button.vue) that encapsulates chipClass, active/has-applied/default states, and the appliedFilterCounts display so other places can reuse it.src/components/searchbox/v2/NodeSearchCategoryTreeNode.vue (1)
11-29: Prefer the shared Button component over raw<button>.This component is under
src/components, so it should use the shared Button for consistent styling/behavior. Consider swapping the native button withsrc/components/ui/button/Button.vuewhile keeping the existing classes and attributes.♻️ Suggested change
<script setup lang="ts"> +import Button from '@/components/ui/button/Button.vue' </script> - <button + <Button type="button" :data-testid="`category-${node.key}`" :aria-current="selectedCategory === node.key || undefined" :class="categoryButtonClass" :style="{ paddingLeft: paddingLeft }" `@click`="$emit('select', node.key)" > <i v-if="showChevrons && hasChildren" :class=" cn( 'pi pi-chevron-down shrink-0 text-[10px] transition-transform', !isExpanded && '-rotate-90' ) " /> {{ node.label }} - </button> + </Button>Based on learnings: In the ComfyUI_frontend Vue codebase, replace raw HTML elements with the shared Button component located at src/components/ui/button/Button.vue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchCategoryTreeNode.vue` around lines 11 - 29, Replace the raw <button> in NodeSearchCategoryTreeNode.vue with the shared Button component (Button.vue): import and register Button, render <Button> with the same attributes/bindings (:data-testid="`category-${node.key}`", :aria-current="selectedCategory === node.key || undefined", :class="categoryButtonClass", :style="{ paddingLeft: paddingLeft }", and `@click`="$emit('select', node.key)"), keep the chevron <i> block and {{ node.label }} as the Button slot content, and preserve the cn usage and reactive props (showChevrons, hasChildren, isExpanded) so behavior and styling remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/searchbox/v2/NodeSearchCategoryTreeNode.vue`:
- Around line 11-29: Replace the raw <button> in NodeSearchCategoryTreeNode.vue
with the shared Button component (Button.vue): import and register Button,
render <Button> with the same attributes/bindings
(:data-testid="`category-${node.key}`", :aria-current="selectedCategory ===
node.key || undefined", :class="categoryButtonClass", :style="{ paddingLeft:
paddingLeft }", and `@click`="$emit('select', node.key)"), keep the chevron <i>
block and {{ node.label }} as the Button slot content, and preserve the cn usage
and reactive props (showChevrons, hasChildren, isExpanded) so behavior and
styling remain identical.
In `@src/components/searchbox/v2/NodeSearchFilterBar.vue`:
- Around line 3-18: The current filter chips render raw <button> elements (see
the v-for over chips, chipClass(chip.key), and `@click`="emit('selectChip',
chip)") which diverges from the codebase guideline to prefer the shared Button
component; either replace each raw button with the shared Button (mapping
aria-pressed, class bindings from chipClass, click handler emit('selectChip',
chip), and the appliedFilterCounts badge) to keep styling/behavior consistent,
or if the three-state visuals are unique, extract this pattern into a reusable
FilterChip component (or add a "chip" variant to
src/components/ui/button/Button.vue) that encapsulates chipClass,
active/has-applied/default states, and the appliedFilterCounts display so other
places can reuse it.
- Category sidebar: nested expanded subfolders wrap in bg-secondary-background container - Chevrons appear on sidebar hover for categories with children - Filter chips: three visual states (active/applied/default) with applied count - Preview card pricing badge truncation for overflow - Pass appliedFilters prop to NodeSearchFilterBar - Fix paddingLeft to preserve depth hierarchy when chevrons are visible - Deduplicate button template in NodeSearchCategoryTreeNode - Restore draftTypes.ts removed by merge conflict with #8993/#8519 Amp-Thread-ID: https://ampcode.com/threads/T-019c87f4-aa28-7290-bdf0-ea5f86aacde3
christian-byrne
force-pushed
the
search-improvements
branch
from
1364170 to
436798d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Designer QA feedback implementation for V2 Node Search added in #8987. Figma reference.
Changes
bg-secondary-backgroundrounded container to indicate their nodes appear in the results listuseElementHover), only for categories with children; no spacing reserved for leaf categoriesbg-base-foreground text-base-background), has-applied-value (bg-base-foreground/10 border-base-foreground), default (border-muted-foreground); shows applied filter count per chipmax-w-full truncatefor overflow handlingappliedFiltersprop fromNodeSearchContenttoNodeSearchFilterBarNodeSearchCategoryTreeNode(was copy-pasted across v-if/v-else)paddingLeftto preserve depth-based indentation when chevrons are visibledraftTypes.tsremoved by merge conflict between chore: remove unused draftTypes.ts to fix knip #8993 and feat(persistence): add draft store and tab state management #8519Review Focus
NodeSearchCategoryTreeNode: usescn()with falsy-returns-undefined to conditionally apply flex/bg classes on a single<div>, avoiding template duplicationbg-base-foreground/text-base-backgroundfor inverted chips matchesbutton.variants.tspattern (no--color-foregroundtoken exists in Tailwind v4 theme)paddingLeftformula:0.5 + depth * 1.25(with chevrons) vs0.75 + depth * 1.25(without) — slightly reduced base to account for chevron icon widthFixes #8987
┆Issue is synchronized with this Notion page by Unito