Feature/1893 data flow diagrams #2389

maiermic · 2021-10-09T14:57:12Z

📑 Summary

Support data flow diagrams by adding brackets [| and |] for datastore nodes to graph, e.g.

Resolves #1893

📏 Design Decisions

I extend graph as data flow diagrams are graphs in general. However, the same grammar is used for flowchart, which does not contain such nodes. I use the brackets [| and |] as suggested by the author of the issue. However, if other nodes may be added in the future that are rectangles with borders/strokes on specific sides, the | indicates rather vertical lines than horizontal. We might consider [- and -] instead. Actually, we could support all sides using brackets:

[| left, but no top stroke
[|- left and top stroke
[- no left, but top stroke
|] right, but no bottom stroke
-|] right and bottom stroke
-] no right, but bottom stroke

However, this would be a larger change/extension of the grammar. Hence, I only added one bracket pair, which is sufficient to cover data flow diagrams. What do you think?

📋 Tasks

Make sure you

📖 have read the contribution guidelines
💻 have added unit/e2e tests (if appropriate)
🔖 targeted develop branch

TODO: Add tests when design decisions are final

See mermaid-js#1893

knsv · 2021-10-14T16:54:09Z

Hello and thanks for looking at this issue.

Have the possibilities to make data flow diagrams would be great and you have made a good start. One thing to keep in mind is backwards compatibility as we can not risk existing authored diagrams to break. This basically means that the syntax once added can never change so no pressure 😄

I like your idea with the brackets which would allow many shapes, do you mean to combine them as well?
[| left, but no top stroke and right and bottom stroke -|]

I don't think [| and |] to represent datastore nodes of data flow diagrams is the best way to go and once in they are there to stay. I think we should go with your syntax as long as it does not break existing diagrams.

The parsing tests are your friendly helper to make sure of that, please add your own as well. It will help you both when working with the grammar and make sure no one else does not break the data flow parsing down the line.

Another think to keep in mind is that graph and the original flow renderer are deprecated and should not be added to. Instead add the shapes so that they render with flowchart (which is the default renderer) using flowRenderer-v2.

I will not merge this right now due to the syntax of [| and |] but hope you want to follow you own idea.

Note that you can call these lexical elements the same thing DF_RECT_START and DF_RECT_STOP and infer the detail semantics of which of the shapes the start and stop symbols form in the flowDB. This will keep the grammar from exploding.

maiermic · 2021-10-14T22:18:13Z

Thanks for the feedback 👍

I like your idea with the brackets which would allow many shapes, do you mean to combine them as well?

Yes, but I just realized that my current brackets do not allow to stroke just right and bottom sides like this

You can use -|] as closing brackets, but what do you use as opening bracket? Maybe just the existing [? But the meaning of [ would depend on the closing bracket, which might not be quite intuitive. The same issue exists for the opposite sides, i.e. left and top.

flowchart LR
  allSides[ stroke all sides ]
  rbSides[ stroke right and bottom sides -|]
  ltSides[|- stroke left and top sides ]

Another issue may be that [- may break existing diagrams, where the node text starts with -, e.g.

flowchart TB
  minusOne[-1]

Till now, it renders as

but due to my suggestion, it would render as

Another idea/solution are bracket-parameters, e.g. with a syntax like this

nodeName [|parameters| node text ]

For example, the letters l (left border), t (top border), r (right border) and b (bottom border) could be allowed:

flowchart LR
  allSides [ stroke all sides ]
  allSides2 [|ltrb| stroke all sides ]
  rbSides [|rb| stroke right and bottom sides ]
  ltSides [|lt| stroke left and top sides ]

Named parameters should be easier to extend, e.g. the same example could be written with named parameters borders as

flowchart LR
  allSides [ stroke all sides ]
  allSides2 [|borders:ltrb| stroke all sides ]
  rbSides [|borders:rb| stroke right and bottom sides ]
  ltSides [|borders:lt| stroke left and top sides ]

An extension could be thick-borders, e.g.

flowchart LR
  node [|borders:lr,thick-borders:tb| thick horizontal, regular vertical ]

What do you think?

knsv · 2021-10-21T16:21:48Z

Sorry for the slow delay. Thursdays is Mermaid night here :)

This looks great! 🚀

I like your last suggestion, go for it!

knsv · 2021-11-03T18:51:04Z

@maiermic This looks great, is the PR still active?

maiermic · 2021-11-03T19:35:36Z

@knsv Yes, I just hadn't the time yet to implement my last suggestion. I'm looking forward to do so.

This reverts commit cd427ab.

maiermic · 2021-11-28T11:42:24Z

dist/dataflowchart.html

+      rbSides[|borders:rb| stroke right and bottom sides ];
+      ltSides[|borders:lt| stroke left and top sides ];
+      lrSides[|borders:lr| stroke left and right sides ];
+      noSide[|borders:no| stroke no side ];


no works, since the word does not contain any of the border letters ltrb. It's not a bug, it's a feature 😉

maiermic · 2021-11-28T11:47:19Z

src/dagre-wrapper/nodes.js

+  if (node.props?.borders) {
+    applyNodePropertyBorders(rect, node.props.borders, totalWidth, totalHeight);
+    propKeys.delete('borders');
+  }


More properties may be added here in the future 🙂

maiermic · 2021-11-28T11:56:38Z

src/dagre-wrapper/nodes.js

+
+  const propKeys = new Set(Object.keys(node.props));
+  if (node.props?.borders) {
+    applyNodePropertyBorders(rect, node.props.borders, totalWidth, totalHeight);


If properties are handled here in this function, only the required data has to be passed to each property handler (here applyNodePropertyBorders). I decided to not extract the whole block (handling all properties) as applyNodeProperties to have access to variables of the current function context (e.g. totalWidth). I could have passed them (e.g. bbox), but it is hard to predict, which variables may be of interest to future property handlers (e.g. that do something else than adding borders). I guess, all variables would be passed to applyNodeProperties in the end.

maiermic · 2021-11-28T12:09:56Z

@knsv I'm confused that the test

XSS
should not allow maniplulating antiscript to run javascript using onerror in state diagrams with dagre wrapper

fails, since xss8.html uses stateDiagram-v2. The changed files in src/diagrams/flowchart/flowDb.js should have no effect. Hence, I guess that it is caused by my changes to src/dagre-wrapper/nodes.js. I'm not sure, how they should break the sanitization. Unless node.props existed before. I assumed that I introduced a new property with this name. Has it been used before?

Edit: I should have looked at the console output first. It does not break the sanitization. It's just an error that is thrown, because node.prop is undefined in src/dagre-wrapper/nodes.js, since it is never set in case of stateDiagram-v2.

maiermic · 2021-11-28T14:06:54Z

src/diagrams/flowchart/parser/flow.jison

@@ -380,6 +381,8 @@ vertex:  idString SQS text SQE
        {$$ = $1;yy.addVertex($1,$3,'stadium');}
    | idString SUBROUTINESTART text SUBROUTINEEND
        {$$ = $1;yy.addVertex($1,$3,'subroutine');}
+    | idString VERTEX_WITH_PROPS_START ALPHA COLON ALPHA PIPE text SQE


I'm not sure if there is a better token (or rule) than ALPHA for the identifier and/or value. It does not allow to add a property thick-borders, but thickBorders would work. That's fine to me. Further, the rule could be changed in the future if required.

knsv · 2021-11-29T06:59:37Z

Great to see this PR progressing. Is it ready for another review?

maiermic · 2021-11-29T09:02:23Z

Yes, it is ready to review

maiermic · 2021-11-29T09:02:48Z

Yes, it is ready to review

knsv · 2021-12-01T17:48:36Z

Thanks for the PR!

StevenIsaacs · 2022-10-10T14:24:09Z

Thank you for the this!

Unfortunately, the implementation is incomplete.

flowchart LR
p((process))
ds[|borders:tb|ds] --> p
ds2[|borders:tb|ds2] --> p
p --> ds2

Renders as:

flowchart LR
  p((process))
  ds[|borders:tb|ds] --> p
  ds2[|borders:tb|ds2] --> p
  p --> ds2

Borders has no effect when ds2 is referenced.

A problem similar to this is: #3263

I added this to that issue.

Michael Maier added 3 commits October 9, 2021 16:19

add brackets [| and |] for datastore nodes of data flow diagrams

cefafba

See mermaid-js#1893

add shape datastore (for nodes of data flow diagram)

cd427ab

See mermaid-js#1893

add example of data flow diagram

e2cd2a7

See mermaid-js#1893

github-actions bot added the feature label Oct 9, 2021

Michael Maier and others added 9 commits November 28, 2021 12:09

replace 'datastore' brackets with 'vertex with props' bracket

dec6b6d

add props to vertex

0550e4a

add props of vertex to node

853d62c

render node property borders of rect node

984a508

update syntax of data flow diagram example

ee3680e

Revert "add shape datastore (for nodes of data flow diagram)"

86810e0

This reverts commit cd427ab.

add borders example

5fff840

Merge branch 'develop' into feature/1893_data_flow_diagrams

9e8c03c

warn if unknown node property is given

b6d3ac8

maiermic commented Nov 28, 2021

View reviewed changes

add missing semicolon

c2db1d4

maiermic commented Nov 28, 2021

View reviewed changes

fix: check if node.prop is defined

28bb07d

knsv merged commit e6cf1f2 into mermaid-js:develop Dec 1, 2021

maiermic mentioned this pull request Oct 10, 2022

fix: border attribute does not work in a certain writing style #3636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/1893 data flow diagrams #2389

Feature/1893 data flow diagrams #2389

maiermic commented Oct 9, 2021

knsv commented Oct 14, 2021

maiermic commented Oct 14, 2021

knsv commented Oct 21, 2021

knsv commented Nov 3, 2021

maiermic commented Nov 3, 2021

maiermic Nov 28, 2021

maiermic Nov 28, 2021

maiermic Nov 28, 2021

maiermic commented Nov 28, 2021 •

edited

Loading

maiermic Nov 28, 2021

knsv commented Nov 29, 2021

maiermic commented Nov 29, 2021

maiermic commented Nov 29, 2021

knsv commented Dec 1, 2021

StevenIsaacs commented Oct 10, 2022

Feature/1893 data flow diagrams #2389

Feature/1893 data flow diagrams #2389

Conversation

maiermic commented Oct 9, 2021

📑 Summary

📏 Design Decisions

📋 Tasks

knsv commented Oct 14, 2021

maiermic commented Oct 14, 2021

knsv commented Oct 21, 2021

knsv commented Nov 3, 2021

maiermic commented Nov 3, 2021

maiermic Nov 28, 2021

Choose a reason for hiding this comment

maiermic Nov 28, 2021

Choose a reason for hiding this comment

maiermic Nov 28, 2021

Choose a reason for hiding this comment

maiermic commented Nov 28, 2021 • edited Loading

maiermic Nov 28, 2021

Choose a reason for hiding this comment

knsv commented Nov 29, 2021

maiermic commented Nov 29, 2021

maiermic commented Nov 29, 2021

knsv commented Dec 1, 2021

StevenIsaacs commented Oct 10, 2022

maiermic commented Nov 28, 2021 •

edited

Loading