Feature/4935 subgraph title margin config option #5041
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #5041 +/- ##
============================================
+ Coverage 42.91% 79.31% +36.39%
============================================
Files 23 166 +143
Lines 5029 13877 +8848
Branches 21 706 +685
============================================
+ Hits 2158 11006 +8848
+ Misses 2870 2719 -151
- Partials 1 152 +151
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Nice work @mathbraga ! Love the tests.
Most of the comments are minor, but we need to dig in why it's not working for basic subgraphs without external connections.
|
@sidharthv96 First time I clicked your link to the live preview I got the same result as shown on your image. Then I checked a second time and now it seems to work all of a sudden.
Edit: Tested again on a fresh browser, the first load renders without the margins, then it fixes itself after refreshing the page. A few other things that I noticed when trying to export the layout. If I choose to copy image to clipboard, download as png or svg, the images are generated correctly with the margins. The other options to create an export link in png, svg, kroki or copy markdown, all generate the image without the margins. |
…tps://github.com/mathbraga/mermaid into feature/4935_subgraph-title-margin-config-option
|
I noticed that setting the 
|
|
@sidharthv96 I'm not sure I understand what you mean. If you set either top or bottom, both will extend the height of the whole subgraph. I implemented this way following the idea on the PR's design decision, where I stretch the subgraph around its children, making it so content stays centered. Should I discard the idea to keep everything centered? I did this just to maintain the layout consistent to the original without margins, and because of the simplicity of implementation, since I don't have to move any of the children nodes, edges, edge labels, etc. Without keeping content centered, it looks something like this (top: 20, bottom:10): Is this what you mean? |
|
I think the extra space at the bottom came due to the workaround used in the issue. If we're adding a margin to the top/bottom of the title, I don't see the need to add extra padding to the bottom of the subgraph itself. In case we need padding for the subgraph itself, they should be exposed as extra options. Is keeping the children centred a requirement I missed? Anyways, this is my personal opinion, we can discuss and finalize. |
|
Keeping content centered is not a requirement. I decided to do so just for consistency with the original layout. So if the original looks centered, so will the layout with margins: But I think I get your point, so just to make sure we are on the same page, from the 2 layouts below, you prefer the one on the left? (both have same margin values) |
|
@mathbraga, yes I think the left one is the more accurate implementation of "Subgraph Title margin", the bottom margin should be a separate config.
|
…tps://github.com/mathbraga/mermaid into feature/4935_subgraph-title-margin-config-option
|
@sidharthv96 It shouldn't have the excessive spacing now. I also added one more test for subgraphs with edge labels and added string templates on a few lines I spotted on a function I was working on. Tell me what you think. |
There was a problem hiding this comment.
Looks great to me! Thanks for adding this feature!
I didn't add anything to the docs since I couldn't find how exactly it works with config changes. I looked around in the code and assumed config docs are generated automatically based on types.
Yep, that's right, they're generated automatically from the schema, so what you've done is perfect! This is what the docs will look like:
Screenshot of auto-generated docs for flowchart.subGraphTitleMargin config
|
@mathbraga, Thank you for the contribution! |
|
@mathbraga I'm afraid this PR has caused a regression. Do you think there is an easy fix for this? |
|
@sidharthv96 The issue with the state diagram is a missplaced parenthesis at line 193. The issue with the sequence diagram are the changes made to sequenceRenderer.ts that came from merging from develop at 42ac630 (these changes seem to have been reverted). So we just need to either move the parenthesis to the right place (right after Do I have to open a new PR? |
|
Can you see the Revert button in #5197 ? |
📑 Summary
Adds the option to introduce top and/or bottom margins for subgraphs title.
Resolves #4935
📏 Design Decisions
The approach is to increase height of subgraphs while keeping its children centered. Doing so creates space to move the
label down which creates the margin. The label only moves down by the value provided for top margin, and everything else moves by the sum of top + bottom margins.
The height of the subgraph is increased by
2 * total marginbecause of how they axisis calculated for nodes.y = node.y - height/2means that if height is increased by twice the margin, the node will move up by the value of total margin, which is compensated in the code withnode.y + total margin, returning the node to the center of its parent.In the code, I chose to move the
y axisof clusters themselves, that way everything inside of the cluster also moves, atleast that's what I understood from the code, so I went for that route for simplicity, other options would require me to also move every child node and edges inside each cluster.This approach fails if a cluster has external connections, since it is considered a normal node with children, meaning it can't be repositioned as a cluster, from what I attempted, interacting with its
y axisonly moves its rectangle and not the whole node. I might be missing something, but from my understanding, all of its child nodes and externally connected nodes have to be repositioned along with every related edge individually. For this reason I asked in #4935 if title margins for this case could be moved to a new separate issue.In summary, if I want to add 10 margin for top and 5 for bottom. I increase height by 30 (15x2), keep children centered, and move subgraph label down by 10 (only top margin):
I didn't add anything to the docs since I couldn't find how exactly it works with config changes. I looked around in the code and assumed config docs are generated automatically based on types.
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.developbranch