Skip to content

Conversation

Myestery
Copy link
Collaborator

@Myestery Myestery commented Aug 21, 2025

This pull request refactors and simplifies the template workflow card components and related UI in the codebase. The main changes focus on removing unused or redundant components, improving visual and interaction consistency, and enhancing error handling for images. Below are the most important changes grouped by theme:

Template Workflow Card Refactor and Cleanup

  • Removed the TemplateWorkflowCard.vue component and its associated test file TemplateWorkflowCard.spec.ts, as well as the TemplateWorkflowCardSkeleton.vue and TemplateWorkflowList.vue components, indicating a shift away from the previous card-based template workflow UI. [1] [2] [3] [4]
  • Removed the TemplateSearchBar.vue component, suggesting a redesign or replacement of the search/filter UI for templates.

UI and Interaction Improvements

  • Improved the CardBottom.vue component by making its height configurable via a fullHeight prop, enhancing layout flexibility.
  • Updated the CardContainer.vue component to add hover effects (background, border, shadow, and padding) and support a new none aspect ratio for more flexible card layouts.

Image and Input Enhancements

  • Enhanced the LazyImage.vue component to display a default placeholder image when an image fails to load, improving error handling and user experience.
  • Improved the SearchBox.vue component by making the input focusable when clicking anywhere on the wrapper, and added a template ref for better accessibility and usability. [1] [2] [3]

Minor UI Tweaks

  • Adjusted label styling in SingleSelect.vue to remove unnecessary overflow handling, simplifying the visual layout.

Copy link

github-actions bot commented Aug 21, 2025

🎭 Playwright Test Results

Some tests failed

⏰ Completed at: 09/26/2025, 05:58:52 PM UTC

📈 Summary

  • Total Tests: 461
  • Passed: 428 ✅
  • Failed: 3 ❌
  • Flaky: 1 ⚠️
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 421 / ❌ 3 / ⚠️ 1 / ⏭️ 29
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

Copy link

github-actions bot commented Aug 21, 2025

🎨 Storybook Build Status

Build completed successfully!

⏰ Completed at: 08/21/2025, 01:43:04 AM UTC

📊 Build Summary

  • Components: 13
  • Stories: 52
  • Visual changes: 0
  • Errors: 0

🔗 Links


🎉 Your Storybook is ready for review!

Copy link
Member

@viva-jinyi viva-jinyi left a comment

Choose a reason for hiding this comment

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

The common components in the input folder shouldn’t be modified. They are presentation components, not container components, and are shared across different places. If options need to be filtered, that logic should be passed down from the place where MultiSelect is used.

@Myestery Myestery force-pushed the feat/new-workflow-templates branch from af4859d to 1be55e7 Compare September 6, 2025 03:44
@comfyui-wiki
Copy link
Member

Custom node's template tag

The custom node's template seems will always has a image tag, need to remove it
image

image

Tag's background color

Can you make the tag's background color slightly darker? When the cover is white, it will be hard to see the text.

image

Getting started title needs loading from index.[locale].json as well

So it can supports i18n as well
image

I18n support is incomplete

image image image image

@christian-byrne
Copy link
Contributor

Can you make the tag's background color slightly darker? When the cover is white, it will be hard to see the text.

We can make a tracking issue and try to solve this one in a future PR

Copy link

socket-security bot commented Sep 24, 2025

All alerts resolved. Learn more about Socket for GitHub.

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report

@Myestery
Copy link
Collaborator Author

I was able to fix the Getting started submenu
However other keys not translated have a separate issue that will be solved by another PR @comfyui-wiki

@Myestery Myestery removed the New Browser Test Expectations New browser test screenshot should be set by github action label Sep 24, 2025
Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

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

The code looks good, I am QA testing now.

'Image API',
'Video API'
])
// Enhanced template interface for easier filtering
Copy link
Contributor

Choose a reason for hiding this comment

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

nit (non-blocking): this could be jsdoc comment so it's available in IDE.

sourceModule: string
category?: string
categoryType?: string
categoryGroup?: string // 'GENERATION TYPE' or 'CLOSED SOURCE MODELS'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit (non-blocking): same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

nit (non-blocking, noting): Generally I think the we should expose errors to the components instead of eating them at the store level. In this case, if an error occurs, we want to throw or expose some reactive error object, then the component is able to decide how to interpret and render that error state in the view layer (e.g., use NoResultsPlaceholder and display the error message set by the store).

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: this one should be in the templates folder as well, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

@christian-byrne
Copy link
Contributor

christian-byrne commented Sep 24, 2025

Notes from testing:

  • I think the "zoom on hover" effect is meant to be applied to the image only (see original implementation from main). I don't think the entire card should be increasing its scale when it is hovered, as it creates a somewhat unsettling animation. Generally a scale change on hover shouldn't result in a layout change that has potential to alter the overall layout (e.g., don't make something grow on hover if it's in flex box or grid as it makes it very awkward to try to navigate the items with your mouse).
  • The button to show tutorial doesn't seem to be using the design system

@christian-byrne
Copy link
Contributor

Found this bug, not sure if it is from this PR or pre-existing:

  • Click the filter selector dropdown
  • Select some items
  • Select the Chat item
  • Unselect the Chat item
  • The dropdown disappears unexpectedly

@christian-byrne
Copy link
Contributor

Another bug related to the image caching:

  1. Go to the Flux sidebar
  2. Change the sort order from default to another one like "Newest"
  3. The items are re-arranged, but the items that were previously out of view are now not loaded (the images) and stuck in the Skeleton state (hypothesis: they start on load on scroll into view events, but it doesn't account for items being re-arranged).

Related: I also wonder if we should increase the intersection observer buffer a bit?

@Myestery
Copy link
Collaborator Author

Another bug related to the image caching:

  1. Go to the Flux sidebar
  2. Change the sort order from default to another one like "Newest"
  3. The items are re-arranged, but the items that were previously out of view are now not loaded (the images) and stuck in the Skeleton state (hypothesis: they start on load on scroll into view events, but it doesn't account for items being re-arranged).

Related: I also wonder if we should increase the intersection observer buffer a bit?

For now I'm re-rendering the templates when the sort changes. we have the images force cached for now using the cache service so there's no mush risk of waiting

@viva-jinyi
Copy link
Member

#5797

Since layout and card style changes are difficult to review purely through code, I went ahead and applied the style adjustments directly in this PR. These changes are to align the UI with the design, so if everything looks good, please go ahead and merge.

Before
before.webm

After
after.webm

@DrJKL
Copy link
Contributor

DrJKL commented Sep 26, 2025

@Myestery Could you post some updated screenshots of the latest version?

@Myestery
Copy link
Collaborator Author

@DrJKL
https://github.com/user-attachments/assets/48b63edc-ee14-438e-9dd4-184778fd09f7

Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

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

LGTM!!

@christian-byrne christian-byrne merged commit d954336 into main Sep 26, 2025
20 of 21 checks passed
@christian-byrne christian-byrne deleted the feat/new-workflow-templates branch September 26, 2025 18:52
christian-byrne added a commit that referenced this pull request Sep 26, 2025
## Summary
- Regenerate 3 Playwright screenshot baselines to reflect UI changes
from #5142
- Also fixed [this
case](https://f01efc75.comfyui-playwright-chromium.pages.dev/#?testId=35f0453d615a452757ca-379124415c5b7e9060d2)
(test case for responsive sizing) as it was using outdated logic. New
logic: ensures that the nav is collapsed on mobile but visible on tablet
and desktop screen sizes. It also ensures that, at all the main
breakpoints, at least 1 card is visible (covers bug that the case was
originally written for wherein the nav was fully covering the cards at
narrow screen widths).

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-5802-ci-Regenerate-Playwright-screenshot-baselines-27a6d73d365081768211da0d24bad2c3)
by [Unito](https://www.unito.io)

---------

Co-authored-by: github-actions <github-actions@github.com>
christian-byrne added a commit that referenced this pull request Sep 27, 2025
This pull request refactors and simplifies the template workflow card
components and related UI in the codebase. The main changes focus on
removing unused or redundant components, improving visual and
interaction consistency, and enhancing error handling for images. Below
are the most important changes grouped by theme:

**Template Workflow Card Refactor and Cleanup**

* Removed the `TemplateWorkflowCard.vue` component and its associated
test file `TemplateWorkflowCard.spec.ts`, as well as the
`TemplateWorkflowCardSkeleton.vue` and `TemplateWorkflowList.vue`
components, indicating a shift away from the previous card-based
template workflow UI.
[[1]](diffhunk://#diff-49569af0404058e8257f3cc0716b066517ce7397dd58744b02aa0d0c61f2a815L1-L139)
[[2]](diffhunk://#diff-9fa6fc1470371f0b520d4deda4129fb313b1bea69888a376556f4bd824f9d751L1-L263)
[[3]](diffhunk://#diff-bc35b6f77d1cee6e86b05d0da80b7bd40013c7a6a97a89706d3bc52573e1c574L1-L30)
[[4]](diffhunk://#diff-48171f792b22022526fca411d3c3a366d48b675dab77943a20846ae079cbaf3bL1-L68)
* Removed the `TemplateSearchBar.vue` component, suggesting a redesign
or replacement of the search/filter UI for templates.

**UI and Interaction Improvements**

* Improved the `CardBottom.vue` component by making its height
configurable via a `fullHeight` prop, enhancing layout flexibility.
* Updated the `CardContainer.vue` component to add hover effects
(background, border, shadow, and padding) and support a new `none`
aspect ratio for more flexible card layouts.

**Image and Input Enhancements**

* Enhanced the `LazyImage.vue` component to display a default
placeholder image when an image fails to load, improving error handling
and user experience.
* Improved the `SearchBox.vue` component by making the input focusable
when clicking anywhere on the wrapper, and added a template ref for
better accessibility and usability.
[[1]](diffhunk://#diff-08f3b0c51fbfe63171509b9944bf7558228f6c2596a1ef5338e88ab64585791bL2-R5)
[[2]](diffhunk://#diff-08f3b0c51fbfe63171509b9944bf7558228f6c2596a1ef5338e88ab64585791bL16-R17)
[[3]](diffhunk://#diff-08f3b0c51fbfe63171509b9944bf7558228f6c2596a1ef5338e88ab64585791bR33-R39)

**Minor UI Tweaks**

* Adjusted label styling in `SingleSelect.vue` to remove unnecessary
overflow handling, simplifying the visual layout.

---------

Co-authored-by: Benjamin Lu <benceruleanlu@proton.me>
Co-authored-by: Alexander Brown <drjkl@comfy.org>
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Christian Byrne <cbyrne@comfy.org>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: snomiao <snomiao@gmail.com>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: filtered <176114999+webfiltered@users.noreply.github.com>
Co-authored-by: Comfy Org PR Bot <snomiao+comfy-pr@gmail.com>
Co-authored-by: Jin Yi <jin12cc@gmail.com>
christian-byrne added a commit that referenced this pull request Sep 27, 2025
## Summary
- Regenerate 3 Playwright screenshot baselines to reflect UI changes
from #5142
- Also fixed [this
case](https://f01efc75.comfyui-playwright-chromium.pages.dev/#?testId=35f0453d615a452757ca-379124415c5b7e9060d2)
(test case for responsive sizing) as it was using outdated logic. New
logic: ensures that the nav is collapsed on mobile but visible on tablet
and desktop screen sizes. It also ensures that, at all the main
breakpoints, at least 1 card is visible (covers bug that the case was
originally written for wherein the nav was fully covering the cards at
narrow screen widths).

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-5802-ci-Regenerate-Playwright-screenshot-baselines-27a6d73d365081768211da0d24bad2c3)
by [Unito](https://www.unito.io)

---------

Co-authored-by: github-actions <github-actions@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:templates size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants