Default Layout

[sroussey]

Set and use a default layout if one or more of the nodes does not have a position.

[clarkmcc]

Couple nits in here that don't really matter and are just suggestions. I'm happy to accept the PR without them. The main thing I'd like to avoid is creating a new array of nodes with useState in the useDefaultLayout thats an exact copy of the ReactFlow internal state. If I'm reading correctly, regardless of whether the default is being used or not, this hook will require that our graph takes 2x the memory required.

I would prefer an approach that avoids this, and if at all possible. For example, I think it would be possible to accomplish everything you're trying to do with this patch. Let me know if I'm overlooking something.

Index: lib/NodeGraphEditor.tsx
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/NodeGraphEditor.tsx b/lib/NodeGraphEditor.tsx
--- a/lib/NodeGraphEditor.tsx	(revision 7fefe87350b6ddabf907ad316ab12656d327e738)
+++ b/lib/NodeGraphEditor.tsx	(date 1706378584380)
@@ -7,6 +7,7 @@
   ReactFlowProps,
   ReactFlowProvider,
   useEdgesState,
+  useNodesInitialized,
   useNodesState,
   useReactFlow,
   useStoreApi,
@@ -23,6 +24,7 @@
   useMemo,
   JSX,
   CSSProperties,
+  useEffect,
 } from 'react'
 import { defaultEdgeTypes } from './edge-types'
 import { IGraphConfig } from './config'
@@ -84,6 +86,10 @@
 
 type FlowProps = ReactFlowProps & {
   backgroundStyles?: CSSProperties
+  /**
+   * The default layout engine to use when nodes are provided without positions.
+   */
+  layoutEngine?: LayoutEngine
 }
 export type NodeGraphHandle = {
   layout: () => void
@@ -91,7 +97,13 @@
 
 const Flow = forwardRef<NodeGraphHandle, FlowProps>(
   (
-    { defaultNodes, defaultEdges, backgroundStyles, ...props }: FlowProps,
+    {
+      defaultNodes,
+      defaultEdges,
+      backgroundStyles,
+      layoutEngine,
+      ...props
+    }: FlowProps,
     ref,
   ) => {
     const nodeTypes = useNodeTypes()
@@ -112,7 +124,7 @@
     )
 
     // Provide methods to parent components
-    const layout = useLayoutEngine(LayoutEngine.Dagre)
+    const layout = useLayoutEngine(layoutEngine ?? LayoutEngine.Dagre)
     useImperativeHandle(
       ref,
       () => ({
@@ -121,6 +133,16 @@
       [],
     )
 
+    const initialized = useNodesInitialized()
+    useEffect(() => {
+      const shouldLayout = !!getState().nodes.find(
+        (node) => node.position == undefined,
+      )
+      if (initialized && shouldLayout) {
+        layout()
+      }
+    }, [initialized])
+
     return (
       <div
         style={{

[sroussey]

That's why it's a draft!

Looks good, I have a couple of other small things, like nodes that have a parent have a new 0,0.

[sroussey]

One reason I called it defaultLayout was because the behavior was meant to act as a layout if positions were lost. I thought I might add layoutEngine separately to force a specific layout even if the nodes had positions from a previous layout. I don't see ever using it, but that was my thinking behind the defaultLayout name.

[sroussey]

Anyhow, this is looks much better than my hack. :)

I am working on my own layouts, I'll keep you updated when I get around to making them clean.

[sroussey]

Also, I am not sure Input Group is the right example to use, it was just available.

[sroussey]

I rebased to main, fyi.

[clarkmcc]

Reasoning is good, defaultLayout is perfectly fine with me. I think I changed it because in my suggested patch it was also the layout engine used by the layout function, but I think long term that method should just accept a layout engine parameter. So yes, defaultLayout makes the most sense.

Yeah I was thinking about creating a new story for it, but there's not much to "show" if you know what I mean. It just works if there are un-positioned nodes, so I'm perfectly fine with having it apply to the input groups story for now if you are.

[clarkmcc]

If you want to rename still, I'll wait to merge

[sroussey]

Naw, I think it's ok as is. 😃