From beb308c5b30b1fedc308bc86bea8b339ad60496c Mon Sep 17 00:00:00 2001
From: rowanfr <beanfun1@gmail.com>
Date: Sun, 7 Jan 2024 05:56:53 -0600
Subject: [PATCH 1/8] Resolved issue with subgraph configuration

This commit resolves the issues with subgraph configuration, specifically for nodeSpacing and rankSpacing. This commit additionally adds an example graph to the `flowchart.html` to demonstrate this resolution. This commit resolves #3258
---
 demos/flowchart.html                        | 24 +++++++++++++++++++++
 packages/mermaid/src/dagre-wrapper/index.js |  4 ++++
 2 files changed, 28 insertions(+)

diff --git a/demos/flowchart.html b/demos/flowchart.html
index d7032a663c..c2e1276fc5 100644
--- a/demos/flowchart.html
+++ b/demos/flowchart.html
@@ -1540,6 +1540,30 @@ <h3>flowchart</h3>
     </pre>
     <hr />
 
+    <pre class="mermaid">
+      ---
+      title: Subgraph nodeSpacing and rankSpacing example
+      ---
+      %%{init:{'flowchart':{'nodeSpacing': 1, 'rankSpacing':'1'}}}%%
+
+      flowchart LR
+      
+      X --> Y
+      
+      subgraph X
+        direction LR
+        A
+        C
+      end
+      
+      subgraph Y
+        direction LR
+        B
+        D
+      end
+    </pre>
+    <hr />
+
     <h1 id="link-clicked">Anchor for "link-clicked" test</h1>
 
     <script type="module">
diff --git a/packages/mermaid/src/dagre-wrapper/index.js b/packages/mermaid/src/dagre-wrapper/index.js
index 76685dd7b9..dfa6db503e 100644
--- a/packages/mermaid/src/dagre-wrapper/index.js
+++ b/packages/mermaid/src/dagre-wrapper/index.js
@@ -54,6 +54,10 @@ const recursiveRender = async (_elem, graph, diagramtype, id, parentCluster, sit
       if (node && node.clusterNode) {
         // const children = graph.children(v);
         log.info('Cluster identified', v, node.width, graph.node(v));
+        // node.graph.setGraph applies the graph configurations such as nodeSpacing to subgraphs as without this the default values would be used
+        node.graph.setGraph(graph.graph()).setDefaultEdgeLabel(function () {
+          return {};
+        });
         const o = await recursiveRender(
           nodes,
           node.graph,

From 3bbd8fe4c9ca08b42e68bbf55b46854b4adb8b82 Mon Sep 17 00:00:00 2001
From: rowanfr <beanfun1@gmail.com>
Date: Sun, 7 Jan 2024 23:08:42 -0600
Subject: [PATCH 2/8] Removed unnecessary function

Removed unnecessary function to pass test coverage CI
---
 packages/mermaid/src/dagre-wrapper/index.js | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/packages/mermaid/src/dagre-wrapper/index.js b/packages/mermaid/src/dagre-wrapper/index.js
index dfa6db503e..b89644f248 100644
--- a/packages/mermaid/src/dagre-wrapper/index.js
+++ b/packages/mermaid/src/dagre-wrapper/index.js
@@ -55,9 +55,7 @@ const recursiveRender = async (_elem, graph, diagramtype, id, parentCluster, sit
         // const children = graph.children(v);
         log.info('Cluster identified', v, node.width, graph.node(v));
         // node.graph.setGraph applies the graph configurations such as nodeSpacing to subgraphs as without this the default values would be used
-        node.graph.setGraph(graph.graph()).setDefaultEdgeLabel(function () {
-          return {};
-        });
+        node.graph.setGraph(graph.graph());
         const o = await recursiveRender(
           nodes,
           node.graph,

From 565f1543c6eb860e54415b3c081e9e511ecd37d8 Mon Sep 17 00:00:00 2001
From: rowanfr <beanfun1@gmail.com>
Date: Sun, 14 Jan 2024 17:12:12 -0600
Subject: [PATCH 3/8] Update demos/flowchart.html

Co-authored-by: Nikolay Rozhkov <nironame@gmail.com>
---
 demos/flowchart.html | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/demos/flowchart.html b/demos/flowchart.html
index c2e1276fc5..147c04edbf 100644
--- a/demos/flowchart.html
+++ b/demos/flowchart.html
@@ -1543,8 +1543,11 @@ <h3>flowchart</h3>
     <pre class="mermaid">
       ---
       title: Subgraph nodeSpacing and rankSpacing example
+      config:
+        flowchart:
+          nodeSpacing: 1
+          rankSpacing: 1
       ---
-      %%{init:{'flowchart':{'nodeSpacing': 1, 'rankSpacing':'1'}}}%%
 
       flowchart LR
       

From f693084065dc092cfee027db633c5eaad0c322c4 Mon Sep 17 00:00:00 2001
From: rowanfr <beanfun1@gmail.com>
Date: Mon, 15 Jan 2024 20:21:01 -0600
Subject: [PATCH 4/8] Fixed configuration override on subgraphs

There was an issue with overriding the subgraphs with the main graphs direction which this commit fixes. It only overrides `rankdir` on the `setGraph` function so that the subgraph preserves it's `rankdir`. There is likely a better way of doing this by modifying the initialization of subgraph configuration to match the main graph when it comes to other elements in the JSON object which composes the graph label.

As we modify the codebase this added line can be deprecated once we add more modular control to subgraphs such as allowing custom spacing or configurations for them. As `rankdir` is the only thing one can set with the `direction` keyword that is the only variable being overwritten for now.
---
 packages/mermaid/src/dagre-wrapper/index.js | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/packages/mermaid/src/dagre-wrapper/index.js b/packages/mermaid/src/dagre-wrapper/index.js
index a74f152795..57ff0d2760 100644
--- a/packages/mermaid/src/dagre-wrapper/index.js
+++ b/packages/mermaid/src/dagre-wrapper/index.js
@@ -53,7 +53,10 @@ const recursiveRender = async (_elem, graph, diagramtype, id, parentCluster) =>
         // const children = graph.children(v);
         log.info('Cluster identified', v, node.width, graph.node(v));
         // node.graph.setGraph applies the graph configurations such as nodeSpacing to subgraphs as without this the default values would be used
-        node.graph.setGraph(graph.graph());
+        // We override the `rankdir` variable to allow for the subgraph's direction to override the graphs direction in the subgraph
+        node.graph.setGraph({...graph.graph(), rankdir: node.graph.graph().rankdir});
+        
+
         const o = await recursiveRender(nodes, node.graph, diagramtype, id, graph.node(v));
         const newEl = o.elem;
         updateNodeBounds(node, newEl);

From 0c7e2e2364f1c242c1fb95dba3dbe6f24aaa4cda Mon Sep 17 00:00:00 2001
From: rowanfr <beanfun1@gmail.com>
Date: Mon, 15 Jan 2024 20:48:29 -0600
Subject: [PATCH 5/8] Fixed linting error

---
 packages/mermaid/src/dagre-wrapper/index.js | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/packages/mermaid/src/dagre-wrapper/index.js b/packages/mermaid/src/dagre-wrapper/index.js
index 57ff0d2760..e0ac9507bf 100644
--- a/packages/mermaid/src/dagre-wrapper/index.js
+++ b/packages/mermaid/src/dagre-wrapper/index.js
@@ -54,8 +54,7 @@ const recursiveRender = async (_elem, graph, diagramtype, id, parentCluster) =>
         log.info('Cluster identified', v, node.width, graph.node(v));
         // node.graph.setGraph applies the graph configurations such as nodeSpacing to subgraphs as without this the default values would be used
         // We override the `rankdir` variable to allow for the subgraph's direction to override the graphs direction in the subgraph
-        node.graph.setGraph({...graph.graph(), rankdir: node.graph.graph().rankdir});
-        
+        node.graph.setGraph({ ...graph.graph(), rankdir: node.graph.graph().rankdir });
 
         const o = await recursiveRender(nodes, node.graph, diagramtype, id, graph.node(v));
         const newEl = o.elem;

From 0ce33cb1601dacc05c6d77f308f4df04f69c6f3f Mon Sep 17 00:00:00 2001
From: rowanfr <beanfun1@gmail.com>
Date: Wed, 28 Feb 2024 13:55:21 -0600
Subject: [PATCH 6/8] Update packages/mermaid/src/dagre-wrapper/index.js

The code setting the `ranksep` and `nodesep` to subgraphs was made cleaner and more legible while maintaining the intended functionality of subgraph spacing

Co-authored-by: Alois Klink <alois@aloisklink.com>
---
 packages/mermaid/src/dagre-wrapper/index.js | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/packages/mermaid/src/dagre-wrapper/index.js b/packages/mermaid/src/dagre-wrapper/index.js
index 8796376fea..e0f7687ff5 100644
--- a/packages/mermaid/src/dagre-wrapper/index.js
+++ b/packages/mermaid/src/dagre-wrapper/index.js
@@ -56,7 +56,12 @@ const recursiveRender = async (_elem, graph, diagramtype, id, parentCluster, sit
         log.info('Cluster identified', v, node.width, graph.node(v));
         // node.graph.setGraph applies the graph configurations such as nodeSpacing to subgraphs as without this the default values would be used
         // We override the `rankdir` variable to allow for the subgraph's direction to override the graphs direction in the subgraph
-        node.graph.setGraph({ ...graph.graph(), rankdir: node.graph.graph().rankdir });
+        const { ranksep, nodesep } = graph.graph();
+        node.graph.setGraph({
+          ...node.graph.graph(),
+          ranksep,
+          nodesep,
+        });
         const o = await recursiveRender(
           nodes,
           node.graph,

From 3602191f698732d0b9aab398ed2cf3c646270536 Mon Sep 17 00:00:00 2001
From: rowanfr <beanfun1@gmail.com>
Date: Wed, 28 Feb 2024 17:12:20 -0600
Subject: [PATCH 7/8] Added E2E testing for spacing

This adds E2E testing for spacing and provides more appropriate comments for the changes made to `index.js`
---
 .../rendering/flowchart-v2.spec.js            | 22 +++++++++++++++++++
 packages/mermaid/src/dagre-wrapper/index.js   |  4 ++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/cypress/integration/rendering/flowchart-v2.spec.js b/cypress/integration/rendering/flowchart-v2.spec.js
index 857d395be7..5cc1f4025b 100644
--- a/cypress/integration/rendering/flowchart-v2.spec.js
+++ b/cypress/integration/rendering/flowchart-v2.spec.js
@@ -760,6 +760,28 @@ A ~~~ B
     );
   });
 
+  it('3258: Should render subgraphs with main graph nodeSpacing and rankSpacing', () => {
+    imgSnapshotTest(
+      `---
+      title: Subgraph nodeSpacing and rankSpacing example
+      ---
+      flowchart LR
+        X --> Y
+        subgraph X
+          direction LR
+          A
+          C
+        end
+        subgraph Y
+          direction LR
+          B
+          D
+        end
+      `,
+      { flowchart: { nodeSpacing: 1, rankSpacing: 1 } }
+    );
+  });
+
   describe('Markdown strings flowchart (#4220)', () => {
     describe('html labels', () => {
       it('With styling and classes', () => {
diff --git a/packages/mermaid/src/dagre-wrapper/index.js b/packages/mermaid/src/dagre-wrapper/index.js
index 0e50d8a3e5..70f1a862cd 100644
--- a/packages/mermaid/src/dagre-wrapper/index.js
+++ b/packages/mermaid/src/dagre-wrapper/index.js
@@ -54,8 +54,8 @@ const recursiveRender = async (_elem, graph, diagramType, id, parentCluster, sit
       if (node && node.clusterNode) {
         // const children = graph.children(v);
         log.info('Cluster identified', v, node.width, graph.node(v));
-        // node.graph.setGraph applies the graph configurations such as nodeSpacing to subgraphs as without this the default values would be used
-        // We override the `rankdir` variable to allow for the subgraph's direction to override the graphs direction in the subgraph
+        // `node.graph.setGraph` applies the graph configurations such as nodeSpacing to subgraphs as without this the default values would be used
+        // We override only the `ranksep` and `nodesep` configurations to allow for setting subgraph spacing while avoiding overriding other properties
         const { ranksep, nodesep } = graph.graph();
         node.graph.setGraph({
           ...node.graph.graph(),

From dbb69ad7cd0d14c9b2f1d97caf7ce535286ffd63 Mon Sep 17 00:00:00 2001
From: Sidharth Vinod <sidharthv96@gmail.com>
Date: Sat, 23 Mar 2024 17:37:45 +0530
Subject: [PATCH 8/8] Add extra test

---
 .../rendering/flowchart-v2.spec.js            | 25 ++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/cypress/integration/rendering/flowchart-v2.spec.js b/cypress/integration/rendering/flowchart-v2.spec.js
index 1318e59649..3eb2a0432a 100644
--- a/cypress/integration/rendering/flowchart-v2.spec.js
+++ b/cypress/integration/rendering/flowchart-v2.spec.js
@@ -773,7 +773,6 @@ A ~~~ B
           C
         end
         subgraph Y
-          direction LR
           B
           D
         end
@@ -782,6 +781,30 @@ A ~~~ B
     );
   });
 
+  it('3258: Should render subgraphs with large nodeSpacing and rankSpacing', () => {
+    imgSnapshotTest(
+      `---
+      title: Subgraph nodeSpacing and rankSpacing example
+      config:
+        flowchart: 
+          nodeSpacing: 250
+          rankSpacing: 250
+      ---
+      flowchart LR
+        X --> Y
+        subgraph X
+          direction LR
+          A
+          C
+        end
+        subgraph Y
+          B
+          D
+        end
+      `
+    );
+  });
+
   describe('Markdown strings flowchart (#4220)', () => {
     describe('html labels', () => {
       it('With styling and classes', () => {