Skip to content

Add Help Menu in NodeLibrarySidebarTab #8179

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

Merged
merged 11 commits into from
Jun 1, 2025

Conversation

benceruleanlu
Copy link
Contributor

@benceruleanlu benceruleanlu commented May 18, 2025

This PR adds a route to serve static web assets from the comfyui-embedded-docs python package.

Related Frontend PR: Comfy-Org/ComfyUI_frontend#3922
Related Docs PR: Comfy-Org/docs#120

Include HELP in node_info

Include HELP in ComfyNodeABC

Readd image to BFL Ultra

Move images to backend
@Amorano
Copy link
Contributor

Amorano commented May 19, 2025

The HELP variable should also accept URL endpoints to dynamically load HTML/MD so that nothing needs to be hard-coded (help info).

@benceruleanlu benceruleanlu marked this pull request as draft May 19, 2025 03:16
@benceruleanlu benceruleanlu marked this pull request as ready for review May 19, 2025 07:50
@benceruleanlu benceruleanlu marked this pull request as draft May 20, 2025 03:01
@Kosinkadink
Copy link
Collaborator

Looks good on the backend PR 👍 Not sure if we should make the logging message be in a similar format as the other 'missing packages' ones, but I see why you have gone with the approach you did since technically missing the docs dep is not critical.

@benceruleanlu
Copy link
Contributor Author

BTW, before merge, version would have to be bumped to whatever is latest, even 0.1.0 is out of date RN.

Copy link
Collaborator

@Kosinkadink Kosinkadink left a comment

Choose a reason for hiding this comment

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

Looks good, we'll just need to sync release with frontend PR once the conflicts are resolved: Comfy-Org/ComfyUI_frontend#3922

Copy link
Collaborator

@webfiltered webfiltered left a comment

Choose a reason for hiding this comment

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

@Kosinkadink It looks like this can be merged ahead of time - do we need to sync frontend release?

Comment on lines +217 to +219
except ImportError:
logging.info("comfyui-embedded-docs package not found")
return None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this have a catch-all that logs and returns? An unhandled exception type here would terminate app start.

Copy link
Collaborator

@Kosinkadink Kosinkadink Jun 1, 2025

Choose a reason for hiding this comment

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

This should be fine - it matches the other import checks in frontend_management; our code checks the return value of this func so it shouldn't be an issue. Assuming the frontend PR handles docs static path not existing, of course:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

And yep, we can definitely merge it head of time.

@Kosinkadink Kosinkadink added Core Core team dependency Core-Important labels Jun 1, 2025
@comfyanonymous comfyanonymous merged commit 180db67 into comfyanonymous:master Jun 1, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Core team dependency Core-Important
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants