-
Notifications
You must be signed in to change notification settings - Fork 143
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
Add a sidebar tab for documentation #1103
base: main
Are you sure you want to change the base?
Conversation
We do have iconify setup now. You can find from a large set of iconify icons here: https://icon-sets.iconify.design/. Here is an example usage in this repo: ComfyUI_frontend/src/components/graph/GraphCanvasMenu.vue Lines 36 to 39 in 39d68bc
|
@@ -344,6 +344,11 @@ const zComfyComboOutput = z.array(z.any()) | |||
const zComfyOutputTypesSpec = z.array( | |||
z.union([zComfyNodeDataType, zComfyComboOutput]) | |||
) | |||
const zDescriptionSpec = z.union([ |
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.
Can you provide links to DESCRIPTION
field used in custom nodes in this format?
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.
A new format is being established here, so no node currently makes use of it. Node packs with a la carte documentation code currently need to play hot potato with swapping out the node def during load so that full descriptions aren't used for node previews or title hover tooltips. There should be a way for a node to describe documentation in place without requiring JS of any form, let alone one that requires abuse of the standards in place.
Test cases have been added for this format.
BTW I think I need to find a way support custom iconify icon. The iconify icon is lazy loaded during compile time so it won't be available for dynamic loaded extensions, but it should be necessary in this situation where in core we want to use iconify icon for sidebar tab icon. |
Related PR: #1169 By making LGraphCanvas shallowReactive, we can observe root object's state directly, and hopefully have the active widget/slot logic handled by litegraph to reduce the overhead of additional event listening. |
The formatting of ndoes using the existing standardized tooltips has been improved. Experiemental work for assisting nodes with display of more detailed descriptions
Previously, description was a simple string, but supporting more complex descriptions requires that new data be passed. The type of a nodes description has been updated to be either a simple string as before, or an array consisting of short description string, an html string for the full description, and a placeholder dict for future usage. Definitions and usage points for description have been updated to accommodate this change
Basic styling has been added to the display of documentation for nodes using the existing tooltip system. This will need another pass to ensure that style updates immediately when the light/dark toggle is hit instead of requiring a change of node. VHS specific namings have been replaced and the code for determining what the mouse is hovering over has been removed. The existing tooltip implementation is cleaner and will need to be integrated anyways so tooltips are temporarily suppressed for the node actively being displayed in the documentation sidebar. Optional callbacks have been added for the initial sidebar display and a user selecting a node element by hovering over it. While selection is not yet implemented, this should cover any developer needs from more involved collapsables to automated seeking to video timestamps.
Basic connecting for using the existing documentation hover code to select an item from the active help pane. Scrolling on selection will now properly perform the minium required scroll to place the element on screen
Styling was moved to the sidebar element for better organization, but this caused errors when the new menu was not in use.
Since the the css is now static the clutter of an added style element is no longer needed
Mostly minor changes to selectors Also fixes the glaringly obvious omission of the description field
Remove errant logging
While I was concerned that doing this would remove the capability to suppress tooltips on the active node, clearing the hoveredItem when it used for documentation functions without even producing a temporary tooltip. A future commit will likely be made so that disabling tooltips for nodes doesn't also prevent the hovered item from being tracked in the store.
The sidebar-documentation branch has diverged enough from main to merging non-trivial Remove deprecated type usage. Move localization to language file.
Not ideal, but implementation is low cost and ensures the displayed documentation properly updates.
Code related to possible options has been pruned. It was pointless for updateNode to take a node as input. Collapsing documentation items does not make sense given the greater space of a sidebar tab compared to a floating window on the graph, so the corresponding code has been fully pruned as well. The topmost node is used instead of the current_node. While current_node displayed the desired properties when canvas was still shallowReactive of notifying a change in node, it's not intended to be used external to litegraph and at best, tracks the topmost visible node instead of the actual topmost node. A test expectation screenshot has been added for verifying theming works for the documentation tab.
afadbcb
to
867c50f
Compare
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.
Can you take a look at the failing tests?
Much belated, but I wanted to find solutions sufficiently clean for core.
Adds a new sidebar tab for displaying documentation information on nodes. Nodes utilizing the existing tooltip functionality will have their descriptions parsed. While this pane is open, hovering over items on the active node will instead scroll to and briefly highlight the corresponding item in the documentation pane. Alternatively, a node can supply a list for its
DESCRIPTION
property which consists ofrender
: A function called when the nodes documentation is displayed.select
: A function called when a node item has been hovered over.Note that both of these current options require a corresponding frontend extension to use. Few nodes will utilize this functionality, but it removes the need for ugly patching on those that do.
Further considerations