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

Sankey diagrams #4502

Merged
merged 87 commits into from
Jul 2, 2023
Merged

Conversation

nirname
Copy link
Contributor

@nirname nirname commented Jun 16, 2023

📑 Summary

Adding new type of diagram - Sankey diagrams

Resolves #4492
I closed First PR in favor of this one, as latter has proper branch name and better description

📏 Design

Sankey diagram represents a flow from one object to another.
GoJS, as well as pages from D3 use csv for that:

source,target,value
Agricultural 'waste',Bio-conversion,124.729
Bio-conversion,Liquid,0.597
Bio-conversion,Losses,26.862
Bio-conversion,Solid,280.322

Instead of heading we expect keyword "sankey".
That is the basic idea.

Lets call these object 'nodes' and these transition 'links'/
We also need a possibility for tuning their attributes, and graph attributes as well, such as:

  • link sort
  • node sort
  • coloring strategy for links (source, target, transition)
  • graph alignment (left, right, width)
  • node color
  • node title
  • node width
  • node padding
  • graph margin

📋 To Do

That is what I plan to do, not everything will have been done at the first approach.

  • 📖 have read the contribution guidelines
  • 🔖 targeted develop branch
  • implement csv syntax
  • implement renderer
  • add demo
  • allow a node to be wrapped in double quotes
  • recognize floating point numbers as values
  • graph with nodes but without flows works
  • remove all comments from graph before parsing (check that this is done automatically, is it related to csv style?)
  • squash some commits and write correct messages for all of them
  • check what has happened with .lock (some deletions, probably undesired) and resolve package json
  • check multiple links between 2 nodes
  • fix conflicts in grammar
  • 💻 have added unit tests
  • fix width (part of the node is cropped at the right)
  • allow to configure height and width
  • 💻 have added e2e tests
  • 📓 have added documentation

@nirname nirname marked this pull request as draft June 16, 2023 22:47
@nirname nirname mentioned this pull request Jun 14, 2023
@nirname nirname force-pushed the feature/4492_sankey-diagrams branch from c8b5d47 to 69897ac Compare June 17, 2023 17:28
@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2023

Codecov Report

Merging #4502 (afb979f) into develop (4570f82) will decrease coverage by 19.60%.
The diff coverage is 86.53%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #4502       +/-   ##
============================================
- Coverage    79.71%   60.11%   -19.60%     
============================================
  Files          137       53       -84     
  Lines        16296     9109     -7187     
  Branches       496       21      -475     
============================================
- Hits         12990     5476     -7514     
- Misses        3208     3633      +425     
+ Partials        98        0       -98     
Flag Coverage Δ
e2e ?
unit 60.11% <86.53%> (+2.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/mermaid/src/diagram-api/diagram-orchestration.ts 30.23% <33.33%> (-9.06%) ⬇️
...ages/mermaid/src/diagrams/sankey/sankeyDetector.ts 75.00% <75.00%> (ø)
packages/mermaid/src/config.type.ts 100.00% <100.00%> (ø)
packages/mermaid/src/defaultConfig.ts 93.48% <100.00%> (-0.33%) ⬇️

... and 130 files with indirect coverage changes

@nirname nirname force-pushed the feature/4492_sankey-diagrams branch from a80e1c9 to 0520fdf Compare June 17, 2023 22:33
@Yokozuna59
Copy link
Member

I'm not a member of the mermaid project, but just to put my two cents in:
 
First of all, thanks for your contribution! Adding more diagrams would really help the mermaid project! I'm aware that you haven't been getting much support or guidance from the community while working with the new diagram, but as you can see, they were busy with other issues.


  • add cypress docker support

I'm not really sure that this should be in this PR; it might be better to have it in a separate PR if necessary. Because adding Docker would mean it should be continuously updated and maintained, I'm not sure if that is a top priority. (correct if I'm wrong)

It might be better to commit with a meaningful message for a better change log; if someone wants to see changes made in a commit, it would be hard with a Wip message.

I haven't seen the code yet, but you could follow new guidelines for diagram definitions in discussed in this PR #4499, info diagram is an example applying those rules.

@nirname
Copy link
Contributor Author

nirname commented Jun 18, 2023

That is how graph looks at the moment
image

@nirname
Copy link
Contributor Author

nirname commented Jun 18, 2023

@Yokozuna59 thanks for reply. Have taken all you'd said into consideration

@sidharthv96
Copy link
Member

@nirname I haven't been much active in Slack. Please try to leave questions and comments related to the PR here, so everyone following the PR has the complete view and can pitch in with suggestions.

I'll also curate a list of new diagram PRs, so people who want to add new diagrams can refer to them, see discussions, and understand why we made some decisions.

The current diagram looks good btw. 🚀

@nirname
Copy link
Contributor Author

nirname commented Jun 18, 2023

@sidharthv96 Thanks a lot

@nirname
Copy link
Contributor Author

nirname commented Jun 18, 2023

Syntax

I have not received any feedback on syntax yet, there is a topic about that #4492, but it does not really matter for me where it is being discussed.
Probably, this feature is not so popular (as many other diagrams), otherwise there would be more people giving feedback on that topic. But this is a good starting point for me.

As for the syntax there are several options (mainly 2). One inspired by dot, and the other by researching on the topic.

Arrow style

(which is the current one)

"Coal"[color=black] // node attributes
"Coal" -> 343.1235 -> "Energy" -> "42.34" -> "Heating" [color=blue] // link attributes

Pros:

  • Allows chaining, that is reasonable and descriptive to my mind
  • We can apply same attributes for many links at once (or not, see attributes in place)
  • Relatively descriptive
  • Ordering of nodes can be setup depending on their order of appearance
  • Duplicating nodes and redefining their attributes may be easily implemented

Cons:

  • may be too complicated and entangled, there is no style recommendation on how to write that
  • we need separate place to specify node attributes (that is an advantage in case of ordering, but probably a disadvantage from different perspective)
  • we cannot define link attributes separately (see attributes in place below)

We can also take a step further and allow defining attributes in place:

"Coal"[color=black] -> 343.1235[color=red] -> "Energy"[color=blue] -> "42.34" -> "Heating" // link and node attributes are mixed

ps
Initially I had an Idea of something like

a -> {
  10 -> b
  12 -> c
} -> 30 -> d -> {10, 20}

but that led to ambiguous behavior so I rejected this option

CSV-style

that is quite often used for that purposes, simply because data is stored that way:

Coal, Energy, 343.1235
Energy, Heating, 42.34

Pros

  • widely used
  • uniform (3 columns always), can be formatted in columns (using spaces)
  • can be copied and paste directly CSV, and extended hereafter
  • It can be even more descriptive, because we see source and target nodes one after another, thereby we can easy perception of what is going on

Cons (well, I dont see any significant):

  • May be it is a little bit excessive, but the worst case scenario for arrow style result in the same length of textual representation code

Fusion

I also think about something heterogeneous, comma based:

a[label="Coal"], b[label="Energy"], 10[color='red']
a[label="Coal"] {
  b[label="Energy"], 10
  e[label="Forging"], 20
}

or arrow based:

a[label="Coal"] -> b[label="Energy"] = 10[color='red']
a[label="Coal"] {
  b[label="Energy"] = 10[color='blue'] // node attributes, link attributes
  e[label="Forging"] = 20[color='red']
} [opacity=50] // common attributes for all links from a to b

Allows pretty much everything that 2 previous styles do, as well as grouping

@nirname nirname requested a review from sidharthv96 June 29, 2023 18:01
Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 more test cases and we're done!

Added cypress container (from their official image),
may be reconsidered later to choose our own
The only minor problem is node JS version mistmatch
package.json 18.16.0
docker-compose mermaid 18.16.1
docker-compose cypress 18.16.0

Host option in cypress docker container must be removed
in favor of possible configuration option.
http://localhost:9000 are currently hard-coded, that is bad

Updated ./run script with better documentation
and added some styles too it as well

Started sankey.spec.js as an example
@nirname
Copy link
Contributor Author

nirname commented Jul 1, 2023

This is last commit message:

Added cypress container (from their official image),
may be reconsidered later to choose our own
The only minor problem is node JS version mistmatch
package.json 18.16.0
docker-compose mermaid 18.16.1
docker-compose cypress 18.16.0

Host option in cypress docker container must be removed
in favor of possible configuration option.
http://localhost:9000 are currently hard-coded, that is bad

Updated ./run script with better documentation
and added some styles too it as well

Started sankey.spec.js as an example (will be reworked)

There was a discussion about the need of adding cypress docker support in this PR, firstly I agreed that it could be postponed but now I've reconsidered my views and would rather stick to my initial proposal. We need this, because it is impossible to write tests without running them locally, so that integration is tightly couple with this PR, because no one before me, obviously, was developing it in Docker

@nirname nirname requested a review from sidharthv96 July 2, 2023 00:20
@nirname nirname marked this pull request as draft July 2, 2023 00:24
@nirname nirname marked this pull request as ready for review July 2, 2023 00:24
@sidharthv96 sidharthv96 added this pull request to the merge queue Jul 2, 2023
@sidharthv96
Copy link
Member

@nirname can you add some more imgSnapshotTests in a different PR?

Merged via the queue into mermaid-js:develop with commit 812e441 Jul 2, 2023
14 of 15 checks passed
@sidharthv96
Copy link
Member

Thanks for the great work @nirname !

Will try to get the release out ASAP.

@sidharthv96
Copy link
Member

sidharthv96 commented Jul 2, 2023 via email

@nirname
Copy link
Contributor Author

nirname commented Jul 2, 2023

@sidharthv96 I have noticed, that this ImgSnapshot test fails sometimes, because cypress discovers some differences in almost 2 identical images (and they really are). I don't know why, If I face the issue I'll attach screenshot. The diff is somewhere near texts, although texts aren't changed, but it still manages to find some imperfect pixels.

By the way, thanks for explanation.

Comment on lines +415 to +426
export enum SankeyLinkColor {
source = 'source',
target = 'target',
gradient = 'gradient',
}

export enum SankeyNodeAlignment {
left = 'left',
right = 'right',
center = 'center',
justify = 'justify',
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nirname, sorry I didn't get a chance to look at this PR before it was merged. The new Sankey diagrams look really good!

TypeScript enums have a bunch of issues, so I've made a PR changing these lines of code to remove the enum, see #4580.

Please feel free to review this PR with your feedback!

github-merge-queue bot pushed a commit to fuxingloh/contented that referenced this pull request Jul 31, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [mermaid](https://togithub.com/mermaid-js/mermaid) | [`10.2.4` ->
`10.3.0`](https://renovatebot.com/diffs/npm/mermaid/10.2.4/10.3.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/mermaid/10.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/mermaid/10.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/mermaid/10.2.4/10.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/mermaid/10.2.4/10.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>mermaid-js/mermaid (mermaid)</summary>

###
[`v10.3.0`](https://togithub.com/mermaid-js/mermaid/releases/tag/v10.3.0):
10.3.0

[Compare
Source](https://togithub.com/mermaid-js/mermaid/compare/v10.2.4...v10.3.0)

#### What's Changed

##### Features

- Sankey diagrams by [@&#8203;nirname](https://togithub.com/nirname) in
[mermaid-js/mermaid#4502
- Feature/1838 actor creation destruction by
[@&#8203;Valentine14th](https://togithub.com/Valentine14th) in
[mermaid-js/mermaid#4466
- Vertical branches in Git Diagram by
[@&#8203;mastersibin](https://togithub.com/mastersibin) in
[mermaid-js/mermaid#4639
- Use JSON Schema to define and document `MermaidConfig` by
[@&#8203;aloisklink](https://togithub.com/aloisklink) in
[mermaid-js/mermaid#4112
- Remove the test checking whether the JSON Schema default config
matched the old default config by
[@&#8203;aloisklink](https://togithub.com/aloisklink) in
[mermaid-js/mermaid#4610
- Fixes support of the macro `ContainerQueue_Ext` for C4 diagrams
definition. by [@&#8203;kislerdm](https://togithub.com/kislerdm) in
[mermaid-js/mermaid#4577

##### Bugfixes

- Make quadrant chart options TypeScript types optional by
[@&#8203;aloisklink](https://togithub.com/aloisklink) in
[mermaid-js/mermaid#4602
- Remove double parsing by
[@&#8203;nirname](https://togithub.com/nirname) in
[mermaid-js/mermaid#4587
- Fix flowchart tooltip typing bug by
[@&#8203;lishid](https://togithub.com/lishid) in
[mermaid-js/mermaid#4562
- Bug/4590 allow notes identical to keywords by
[@&#8203;ibrahimWassouf](https://togithub.com/ibrahimWassouf) in
[mermaid-js/mermaid#4597
- feat: allow specifying on which weekday a tickInterval should start by
[@&#8203;leinelissen](https://togithub.com/leinelissen) in
[mermaid-js/mermaid#4634
- Split formatted markdown strings with unicode support. by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[mermaid-js/mermaid#4470
- fix: Mind maps handles `-` signs in node ids/text by
[@&#8203;knsv](https://togithub.com/knsv)

##### Chores

- Remove all TypeScript enums and forbid them in ESLint by
[@&#8203;aloisklink](https://togithub.com/aloisklink) in
[mermaid-js/mermaid#4580
- refactor accessibility by
[@&#8203;Yokozuna59](https://togithub.com/Yokozuna59) in
[mermaid-js/mermaid#4551
- chore: Reduce codecov pushes by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[mermaid-js/mermaid#4604
- Run PR-labeler-config-validator only if config changes by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[mermaid-js/mermaid#4607
- chore(deps): update all minor dependencies (minor) by
[@&#8203;renovate](https://togithub.com/renovate) in
[mermaid-js/mermaid#4624
- Update all patch dependencies (patch) by
[@&#8203;renovate](https://togithub.com/renovate) in
[mermaid-js/mermaid#4566
- Update all patch dependencies (patch) by
[@&#8203;renovate](https://togithub.com/renovate) in
[mermaid-js/mermaid#4581
- Rename workflow jobs by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[mermaid-js/mermaid#4574
- Removed unused code in state diagrams by
[@&#8203;nirname](https://togithub.com/nirname) in
[mermaid-js/mermaid#4631
- chore(deps): update all patch dependencies (patch) by
[@&#8203;renovate](https://togithub.com/renovate) in
[mermaid-js/mermaid#4623
- chore: remove unused `devDependency` on coveralls by
[@&#8203;aloisklink](https://togithub.com/aloisklink) in
[mermaid-js/mermaid#4641
- Allow entity diagram attribute names to start with asterisk by
[@&#8203;ibrahimWassouf](https://togithub.com/ibrahimWassouf) in
[mermaid-js/mermaid#4588
- Bug/4592 fix new line padding class diagram by
[@&#8203;ibrahimWassouf](https://togithub.com/ibrahimWassouf) in
[mermaid-js/mermaid#4633
- Fix graph not loading when the img loads too fast or fail to load by
[@&#8203;pierrickouw](https://togithub.com/pierrickouw) in
[mermaid-js/mermaid#4496
- convert `cypress/helpers/util.js` to ts by
[@&#8203;Yokozuna59](https://togithub.com/Yokozuna59) in
[mermaid-js/mermaid#4552
- build(deps-dev): bump word-wrap from 1.2.3 to 1.2.4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[mermaid-js/mermaid#4652
- chore(deps): update all minor dependencies (minor) by
[@&#8203;renovate](https://togithub.com/renovate) in
[mermaid-js/mermaid#4663
- chore(deps): update all patch dependencies (patch) by
[@&#8203;renovate](https://togithub.com/renovate) in
[mermaid-js/mermaid#4662

##### Documentation

- Sankey: Remove duplicated examples by
[@&#8203;nirname](https://togithub.com/nirname) in
[mermaid-js/mermaid#4595
- Release docs by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[mermaid-js/mermaid#4493
- Update latest news section by
[@&#8203;huynhicode](https://togithub.com/huynhicode) in
[mermaid-js/mermaid#4495
- Fix Typo by [@&#8203;ryru](https://togithub.com/ryru) in
[mermaid-js/mermaid#4567
- Docs: add ChatGPT plugin blog post by
[@&#8203;huynhicode](https://togithub.com/huynhicode) in
[mermaid-js/mermaid#4570
- Fix relative link to theme variables list by
[@&#8203;ibrahimWassouf](https://togithub.com/ibrahimWassouf) in
[mermaid-js/mermaid#4573
- Fix docs:dev by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[mermaid-js/mermaid#4598
- Docs: update link - "Join the Community" by
[@&#8203;huynhicode](https://togithub.com/huynhicode) in
[mermaid-js/mermaid#4601
- Support docs:dev in docker by
[@&#8203;nirname](https://togithub.com/nirname) in
[mermaid-js/mermaid#4599
- docs(flowchart): add documentation on multiple nodes style by
[@&#8203;tomperr](https://togithub.com/tomperr) in
[mermaid-js/mermaid#4600
- Avoid downloading avtars everytime on docs:dev by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[mermaid-js/mermaid#4603
- docs: Fix checkbox syntax by
[@&#8203;guilhermgonzaga](https://togithub.com/guilhermgonzaga) in
[mermaid-js/mermaid#4646
- Fix the "Edit this page on GitHub" link in Vitepress documentation for
the Mermaid Config pages by
[@&#8203;aloisklink](https://togithub.com/aloisklink) in
[mermaid-js/mermaid#4640
- Support MERMAID_RELEASE_VERSION in docs. by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[mermaid-js/mermaid#4612
- Docs: update Latest News section by
[@&#8203;huynhicode](https://togithub.com/huynhicode) in
[mermaid-js/mermaid#4655
- added Typora to integrations list by
[@&#8203;kgilbert78](https://togithub.com/kgilbert78) in
[mermaid-js/mermaid#4666
- Docs: Corrects name of C4 link by
[@&#8203;Incognito](https://togithub.com/Incognito) in
[mermaid-js/mermaid#4660
- Fix a typo by [@&#8203;gjtorikian](https://togithub.com/gjtorikian) in
[mermaid-js/mermaid#4396

#### New Contributors

- [@&#8203;ryru](https://togithub.com/ryru) made their first
contribution in
[mermaid-js/mermaid#4567
- [@&#8203;ibrahimWassouf](https://togithub.com/ibrahimWassouf) made
their first contribution in
[mermaid-js/mermaid#4573
- [@&#8203;kislerdm](https://togithub.com/kislerdm) made their first
contribution in
[mermaid-js/mermaid#4577
- [@&#8203;leinelissen](https://togithub.com/leinelissen) made their
first contribution in
[mermaid-js/mermaid#4634
- [@&#8203;pierrickouw](https://togithub.com/pierrickouw) made their
first contribution in
[mermaid-js/mermaid#4496
- [@&#8203;mastersibin](https://togithub.com/mastersibin) made their
first contribution in
[mermaid-js/mermaid#4639
- [@&#8203;kgilbert78](https://togithub.com/kgilbert78) made their first
contribution in
[mermaid-js/mermaid#4666
- [@&#8203;Incognito](https://togithub.com/Incognito) made their first
contribution in
[mermaid-js/mermaid#4660
- [@&#8203;gjtorikian](https://togithub.com/gjtorikian) made their first
contribution in
[mermaid-js/mermaid#4396

**Full Changelog**:
mermaid-js/mermaid@v10.2.4...v10.3.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/levaintech/contented).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMS4wIiwidXBkYXRlZEluVmVyIjoiMzYuMTEuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sankey diagrams
5 participants