Flowchart: Apply nodeSpacing/rankSpacing config to subgraphs
#5183
Conversation
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 mermaid-js#3258
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5183 +/- ##
==========================================
- Coverage 5.74% 5.74% -0.01%
==========================================
Files 277 277
Lines 41743 41751 +8
Branches 27 27
==========================================
Hits 2400 2400
- Misses 39342 39350 +8
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Removed unnecessary function to pass test coverage CI
Co-authored-by: Nikolay Rozhkov <nironame@gmail.com>
|
@nirname Thanks for the suggested change in syntax, I didn't even know that was available |
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.
This comment was marked as resolved.
This comment was marked as resolved.
|
@sidharthv96 Could you tell me how or where you encountered the errors that your showing? I'm using the dev server to get output data and they all seem to be resolving as expected. I'm unfortunately unfamiliar with cypress so I don't know how to run it locally. |
|
It was observed after the run of the E2E test that was triggered by the pull request event (not the push event). |
|
After some research the e2e test failing doesn't seem to occur locally outside of randomly when changing the window or running other applications (and even this doesn't align with the CI tests which fail), however Cypress errors for |
|
For the examples above all seem to have the links and graphs shift position slightly. This seems to be an browser dependent issue as I can test locally on electron it repeatedly produce successful graphs for Cypress, firefox nightly seems to randomly produce successful graphs while edge and chrome do not. As I've stated earlier I'll look into this a bit further but I think this has to do with browser rendering diff, which I don't have the ability to address |
|
Hmm, The E2E tests are working fine on other PRs, so it means some change in this one is triggering the browser inconsistency, and it's failing consistently as well, not flaky. Maybe your change has some other effects downstream that's causing the issue. We'll have to investigate further. @aloisklink @nirname @Yokozuna59 do you folks have any pointers? |
There was a problem hiding this comment.
I think I've spotted the issue that was causing the E2E tests to fail, we just need to slightly modify the code so that it doesn't copy the parent graph's margin.
It would also be nice to have a very basic E2E test in the cypress/integration/rendering/flowchart-v2.spec.js file (e.g. your demo diagram), just so we get automated tests so we know not to break your feature!
Other than that, this PR looks good to me. Awesome work in finding the place to update in the first place, the packages/mermaid/src/dagre-wrapper/index.js file is pretty difficult to understand!
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>
This adds E2E testing for spacing and provides more appropriate comments for the changes made to `index.js`
|
@aloisklink Thanks for spotting the issue that was causing the E2E tests to fail. That problem genuinely had me completely stumped for months. I've added the E2E testing for subgraph configuration as you suggested |
aloisklink
changed the title
nodeSpacing/rankSpacing config to subgraphs
There was a problem hiding this comment.
Looks awesome to me now! Btw, I've slightly adjusted the PR title, let me know if you don't like it and want to change it back (or to something else).
Thanks for spotting the issue that was causing the E2E tests to fail. That problem genuinely had me completely stumped for months.
Yep, the dagre-wrapper/index.js file is really really hard to read. I really want to look into converting it to TypeScript and adding at least some documentation to it, but I think we might also need to fix some types in a third-party library (and that library is also very undocumented), so it would be a lot of work :(
aloisklink
added
Type: Bug / Error
|
The new title looks great and is more accurate to the solution, the earlier title was generally what was desired so the change to it makes sense. As for |
|
@nirname could you check over the changes you've requested to see if they've been completed and then approve the PR if you feel confident? |
|
Is there an update to this pull request, I am having the same issues and would love a solution :) |
|
@Stevio54 It's feature complete, it just needs to be merged |
📑 Summary
This commit resolves the issues with subgraph configuration, specifically for
nodeSpacingandrankSpacing. This commit additionally adds an example graph to theflowchart.htmlto demonstrate this resolution.Resolves #3258
📏 Design Decisions
This is a fairly simple modification that applies the configuration held in the label for the overall graph to the configuration for the cluster/subgraph in order to ensure they have matching configurations. This is done right before
recursiveRenderbegins to render each cluster. The reason this needs to be done is that the label for the subgraphs do not contain the correctnodeSpacingandrankSpacingbased on the mermaid configuration. Therefore if one overwrites the graph label right before rendering it resolves the mismatch of configurations.Some potential improvements for this commit would be to allow subgraphs to have there own customization labels. This would be fairly trivial (presumably) by just replacing
graph.graph()with whatever settings you want from whatever configurations you want.📋 Tasks
Make sure you
💻 have added necessary unit/e2e tests. (Not necessary to my knowledge though I'm open to alternative opinions)
📓 have added documentation. Make sure
MERMAID_RELEASE_VERSIONis used for all new features. (Not necessary to my knowledge as this is a bugfix)developbranch