Skip to content
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

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

AustinMroz
Copy link
Collaborator

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 of

  • A short description which is used for node previews and title tooltips
  • A full html description
  • An optional dict to facilitate data transfer or provide additional configuration
    • render: 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

  • Having NodeTooltips import the callback directly from documentationSidebar causes documentationSidebar to load before extensionManager is set in app. The current implementation allows this tooltipCallback to be hooked by other extensions, but seems the cleanest implementation.
  • I quite dislike the question mark icon supplied by prime icons and implement an alternative with css. Mention was made of allowing icons other than those supplied by prime icons, but adding a generalized implementation here would be out of scope.

@huchenlei
Copy link
Member

I quite dislike the question mark icon supplied by prime icons and implement an alternative with css. Mention was made of allowing icons other than those supplied by prime icons, but adding a generalized implementation here would be out of scope.

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:

<template #icon>
<i-material-symbols:pan-tool-outline v-if="canvasStore.readOnly" />
<i-simple-line-icons:cursor v-else />
</template>

@huchenlei huchenlei self-requested a review October 4, 2024 19:08
src/assets/css/style.css Outdated Show resolved Hide resolved
src/extensions/core/documentationSidebar.ts Outdated Show resolved Hide resolved
@@ -344,6 +344,11 @@ const zComfyComboOutput = z.array(z.any())
const zComfyOutputTypesSpec = z.array(
z.union([zComfyNodeDataType, zComfyComboOutput])
)
const zDescriptionSpec = z.union([
Copy link
Member

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?

Copy link
Collaborator Author

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.

src/types/comfy.d.ts Outdated Show resolved Hide resolved
src/components/graph/NodeTooltip.vue Outdated Show resolved Hide resolved
@huchenlei
Copy link
Member

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:

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.

@huchenlei
Copy link
Member

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
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.
@AustinMroz AustinMroz force-pushed the sidebar-documentation branch from afadbcb to 867c50f Compare November 28, 2024 17:26
Copy link
Member

@huchenlei huchenlei left a 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants