Fix edge handling and arrow type #4582

jgreywolf · 2023-07-03T00:51:44Z

📑 Summary

Adds realization markers to class Diagram.

Resolves #3816
Resolves #1687

📏 Design Decisions

📋 Tasks

Make sure you

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

codecov · 2023-07-03T00:51:58Z

Codecov Report

Merging #4582 (de6add6) into develop (ebaabbf) will increase coverage by 28.21%.
Report is 3 commits behind head on develop.
The diff coverage is 80.00%.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #4582       +/-   ##
============================================
+ Coverage    45.25%   73.46%   +28.21%     
============================================
  Files           53      146       +93     
  Lines         6654    14526     +7872     
  Branches        32      615      +583     
============================================
+ Hits          3011    10671     +7660     
- Misses        3642     3710       +68     
- Partials         1      145      +144

Flag	Coverage Δ
e2e	`79.22% <80.00%> (?)`
unit	`45.16% <ø> (-0.09%)`	⬇️

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

Files Changed	Coverage Δ
packages/mermaid/src/diagrams/class/classDb.ts	`82.44% <ø> (ø)`
packages/mermaid/src/diagrams/class/styles.js	`100.00% <ø> (ø)`
packages/mermaid/src/dagre-wrapper/edges.js	`77.56% <50.00%> (ø)`
packages/mermaid/src/dagre-wrapper/markers.js	`100.00% <100.00%> (ø)`
...ges/mermaid/src/diagrams/class/classRenderer-v2.ts	`80.14% <100.00%> (ø)`

... and 137 files with indirect coverage changes

aloisklink

A couple of issues (although they seem pretty minor, so I wouldn't mind if it's just documented somewhere that these are purposely not covered to make implementation easier):

What if the background isn't using the CSS background-color property, but is instead using something like background: green or background-image: url(...)?
What happens if the background is a transparent color (e.g. rgba(255, 0, 0, 0.2)?

Ideally, we'd figure out how to stop drawing the line under the arrow head, but I'm not an SVG expert, so I've got no idea how hard that would be. It might not be worth the effort just to handle a couple of rare edge cases 🤷

packages/mermaid/src/dagre-wrapper/markers.js

sidharthv96 · 2023-07-03T13:14:55Z

Tangent: Should we add some performance benchmarks?
Something that integrates into imgsnapshot tests would make sure most cases are covered, but we'll also need some tests with multiple iterations.

Then we can have a reporter which comments the performance difference on each PR.

sidharthv96

Like Alois said, stopping the line before the arrowhead is the best solution if possible.

packages/mermaid/src/dagre-wrapper/markers.js

Co-authored-by: Alois Klink <alois@aloisklink.com>

jgreywolf · 2023-07-03T15:58:50Z

A couple of issues (although they seem pretty minor, so I wouldn't mind if it's just documented somewhere that these are purposely not covered to make implementation easier):
* What if the background isn't using the CSS `background-color` property, but is instead using something like `background: green` or `background-image: url(...)`?

* What happens if the background is a transparent color (e.g. `rgba(255, 0, 0, 0.2)`?
Ideally, we'd figure out how to stop drawing the line under the arrow head, but I'm not an SVG expert, so I've got no idea how hard that would be. It might not be worth the effort just to handle a couple of rare edge cases 🤷

It doesnt matter if the background isnt using a CSS property. the code is grabbing the "computed" styles - so if there is a background at all this method will pull it out.
And if it is transparent I am just setting it to white

As to not drawing the line. Totally agreed. I started looking into this and have not figure out how to adjust this yet

aloisklink · 2023-07-05T18:28:05Z

And if it is transparent I am just setting it to white

I think the code currently only checks for 100% white transparency, aka rgba(0, 0, 0, 0), see

mermaid/packages/mermaid/src/dagre-wrapper/markers.js

Line 39 in 65c0c0a

return backgroundColor === 'rgba(0, 0, 0, 0)' ? 'white' : backgroundColor;

I'm not sure what it does if you have partial transparencies (e.g. rgba(255, 0, 0, 0.2), which is a red with 80% transparency (aka pink if the background is white)).

As to not drawing the line. Totally agreed. I started looking into this and have not figure out how to adjust this yet

If it's too difficult, this background hack should still handle 99.9% of cases. We just need to document somewhere that this is a hack, why the hack exists, and the limitations it has. E.g. something like:

/**
 * Finds the parent background color of an SVG element.
 *
 * Used to make a "hollow" arrowhead for an arrow.
 *
 * We can't use a transparent fill,
 * because the arrow line is behind the arrowhead
 * (ideally we'd stop drawing the line behind the arrowhead,
 *  but this is pretty complicated to do). 
 *
 * **Limitations**:
 * - If the parent background color is a partial transparency,
 *   the arrowhead will also be partially transparent.
 * - More complicated backgrounds like pictures/animations won't work.
 */
const getBackgroundColor = (elem) => {

We could always figure out not drawing the line under the arrowheads in the future.

Honestly, most class UML diagrams just pick white for hollow arrows, so picking a background color is already an improvement, e.g. like:

jgreywolf · 2023-07-11T01:37:28Z

And if it is transparent I am just setting it to white

I think the code currently only checks for 100% white transparency, aka rgba(0, 0, 0, 0), see

mermaid/packages/mermaid/src/dagre-wrapper/markers.js

Line 39 in 65c0c0a

return backgroundColor === 'rgba(0, 0, 0, 0)' ? 'white' : backgroundColor;

I'm not sure what it does if you have partial transparencies (e.g. rgba(255, 0, 0, 0.2), which is a red with 80% transparency (aka pink if the background is white)).

As to not drawing the line. Totally agreed. I started looking into this and have not figure out how to adjust this yet

If it's too difficult, this background hack should still handle 99.9% of cases. We just need to document somewhere that this is a hack, why the hack exists, and the limitations it has. E.g. something like:
/**
 * Finds the parent background color of an SVG element.
 *
 * Used to make a "hollow" arrowhead for an arrow.
 *
 * We can't use a transparent fill,
 * because the arrow line is behind the arrowhead
 * (ideally we'd stop drawing the line behind the arrowhead,
 *  but this is pretty complicated to do). 
 *
 * **Limitations**:
 * - If the parent background color is a partial transparency,
 *   the arrowhead will also be partially transparent.
 * - More complicated backgrounds like pictures/animations won't work.
 */
const getBackgroundColor = (elem) => {
We could always figure out not drawing the line under the arrowheads in the future.

Honestly, most class UML diagrams just pick white for hollow arrows, so picking a background color is already an improvement, e.g. like:

Ill add the comment :)

netlify · 2023-08-26T22:09:47Z

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	`de6add6`
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/64f808381c1b09000879b946
😎 Deploy Preview	https://deploy-preview-4582--mermaid-js.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jgreywolf · 2023-08-26T22:17:12Z

Didnt realize this was still pending

aloisklink

Looks good to me!

Is it worth adding some of limitations described in #4582 (comment) into the PR description?

packages/mermaid/src/dagre-wrapper/markers.js

Co-authored-by: Alois Klink <alois@aloisklink.com>

packages/mermaid/src/dagre-wrapper/markers.js

sidharthv96 · 2023-08-28T18:48:25Z

In dark mode, white looks wrong. What can be done here?

sidharthv96 · 2023-08-28T19:19:41Z

By tweaking the linefunction and refX, we can actually achive proper transparent markers. It will require some extra handling of cases, but I think we can avoid the DOM walking, and the white fallback.

diff --git a/packages/mermaid/src/dagre-wrapper/edges.js b/packages/mermaid/src/dagre-wrapper/edges.js
index 1581658b0..81c79ee5d 100644
--- a/packages/mermaid/src/dagre-wrapper/edges.js
+++ b/packages/mermaid/src/dagre-wrapper/edges.js
@@ -450,7 +450,7 @@ export const insertEdge = function (elem, e, edge, clusterDb, diagramType, graph
       return d.x;
     })
     .y(function (d) {
-      return d.y;
+      return d.y - 18;
     })
     .curve(curve);
 
diff --git a/packages/mermaid/src/dagre-wrapper/markers.js b/packages/mermaid/src/dagre-wrapper/markers.js
index 57d092fdf..13ac4cf55 100644
--- a/packages/mermaid/src/dagre-wrapper/markers.js
+++ b/packages/mermaid/src/dagre-wrapper/markers.js
@@ -57,7 +57,7 @@ const composition = (elem, type) => {
     .append('marker')
     .attr('id', type + '-compositionEnd')
     .attr('class', 'marker composition ' + type)
-    .attr('refX', 19)
+    .attr('refX', 1)
     .attr('refY', 7)
     .attr('markerWidth', 20)
     .attr('markerHeight', 28)
diff --git a/packages/mermaid/src/diagrams/class/styles.js b/packages/mermaid/src/diagrams/class/styles.js
index 15386bf9e..1a391ae53 100644
--- a/packages/mermaid/src/diagrams/class/styles.js
+++ b/packages/mermaid/src/diagrams/class/styles.js
@@ -91,7 +91,7 @@ g.classGroup line {
 }
 
 #compositionEnd, .composition {
-  fill: ${options.lineColor} !important;
+  fill: transparent !important;
   stroke: ${options.lineColor} !important;
   stroke-width: 1;
 }

jgreywolf · 2023-08-28T20:00:37Z

By tweaking the linefunction and refX, we can actually achive proper transparent markers. It will require some extra handling of cases, but I think we can avoid the DOM walking, and the white fallback.

But what determines those are the right values to use? magic numbers...

sidharthv96 · 2023-08-29T04:02:42Z

The values to use needs to be computed for the marker types. It's based on the Ref values they currently have.
The only remaining problem is when nodes have an angle. Let me see if that can be handled.

This handles the start and end link shrinking, based on what kind the markers are.

sidharthv96 · 2023-08-29T08:43:38Z

Almost done, a little tweak needed for minor adjustment of the offset.

* develop: (56 commits) chore: Fix unit tests chore(deps): update all patch dependencies chore: Update docs Update docs New Mermaid Live Editor for Confluence Cloud (#4814) Update link to Discourse theme component (#4811) Update flowchart.md (#4810) chore: remove unneeded `CommomDB` chore: Update docs "CSS" instead of "css" in flowchart.md (#4797) Update CONTRIBUTING.md Update CONTRIBUTING.md fix: typos (#4801) chore: Align with convention chore: Add JSDoc to apply in sequenceDB refactor: Tidy up direction handling chore: Fix flowchart arrow chore: Add test to verify activate chore: Update tests snapshot fix: #4691 Align arrowheads properly in sequenceDiagram ...

sidharthv96 · 2023-09-06T05:09:37Z

packages/mermaid/src/diagrams/class/classDiagram.spec.ts

-      const str = 'classDiagram\n' + 'Class1 --|> Class02';
+      const str = 'classDiagram\n' + 'Class1 --|> Class2';


We already support Extension --|> and Realization ..|>.
Wouldn't releasing this change mark the extension as a realization, changing the meaning of a lot of existing diagrams?

classDiagram classA <|-- classB classM <|.. classN

Loading

jgreywolf · 2023-09-08T00:35:46Z

I am going to close this PR, and redo to take into account the fix with #4788

jgreywolf added 2 commits July 2, 2023 17:44

add implementation/realization edge type, fix arrow heads to be hollow

6d07941

Merge branch 'develop' into UpdateClasstoMatchUmlSpecs

ada4ddc

jgreywolf requested review from sidharthv96 and aloisklink July 3, 2023 00:52

aloisklink reviewed Jul 3, 2023

View reviewed changes

packages/mermaid/src/dagre-wrapper/markers.js Outdated Show resolved Hide resolved

sidharthv96 reviewed Jul 3, 2023

View reviewed changes

packages/mermaid/src/dagre-wrapper/markers.js Outdated Show resolved Hide resolved

Update packages/mermaid/src/dagre-wrapper/markers.js

824feb5

Co-authored-by: Alois Klink <alois@aloisklink.com>

jgreywolf added 3 commits July 3, 2023 09:41

Merge branch 'develop' into UpdateClasstoMatchUmlSpecs

7a987a2

prettier fixes

96e5151

small test fix

65c0c0a

jgreywolf added 2 commits August 26, 2023 14:08

Merge branch 'develop' into UpdateClasstoMatchUmlSpecs

dc1308a

add comment

c323293

jgreywolf marked this pull request as ready for review August 26, 2023 22:16

aloisklink added the Type: Bug / Error Something isn't working or is incorrect label Aug 27, 2023

aloisklink approved these changes Aug 27, 2023

View reviewed changes

packages/mermaid/src/dagre-wrapper/markers.js Outdated Show resolved Hide resolved

aloisklink added the Graph: Class label Aug 27, 2023

Update packages/mermaid/src/dagre-wrapper/markers.js

67ff83a

Co-authored-by: Alois Klink <alois@aloisklink.com>

jgreywolf mentioned this pull request Aug 28, 2023

3952 - lexical ids #4785

Closed

jgreywolf linked an issue Aug 28, 2023 that may be closed by this pull request

Documentation for classDiagram examples: "attribute type" order is backwards compared to UML #4489

Closed

sidharthv96 approved these changes Aug 28, 2023

View reviewed changes

packages/mermaid/src/dagre-wrapper/markers.js Outdated Show resolved Hide resolved

sidharthv96 mentioned this pull request Aug 29, 2023

fix: Allow hollow markers on edges #4788

Merged

4 tasks

sidharthv96 added 2 commits September 5, 2023 23:36

chore: Remove background color logic

de6add6

sidharthv96 requested changes Sep 6, 2023

View reviewed changes

jgreywolf closed this Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix edge handling and arrow type #4582

Fix edge handling and arrow type #4582

jgreywolf commented Jul 3, 2023 •

edited by sidharthv96

Loading

codecov bot commented Jul 3, 2023 •

edited

Loading

aloisklink left a comment

sidharthv96 commented Jul 3, 2023 •

edited

Loading

sidharthv96 left a comment

jgreywolf commented Jul 3, 2023

aloisklink commented Jul 5, 2023 •

edited

Loading

jgreywolf commented Jul 11, 2023

netlify bot commented Aug 26, 2023 •

edited

Loading

jgreywolf commented Aug 26, 2023

aloisklink left a comment

sidharthv96 commented Aug 28, 2023

sidharthv96 commented Aug 28, 2023 •

edited

Loading

jgreywolf commented Aug 28, 2023

sidharthv96 commented Aug 29, 2023 •

edited

Loading

sidharthv96 commented Aug 29, 2023

sidharthv96 Sep 6, 2023

jgreywolf commented Sep 8, 2023

		const str = 'classDiagram\n' + 'Class1 --\|> Class02';
		const str = 'classDiagram\n' + 'Class1 --\|> Class2';

Fix edge handling and arrow type #4582

Fix edge handling and arrow type #4582

Conversation

jgreywolf commented Jul 3, 2023 • edited by sidharthv96 Loading

📑 Summary

📏 Design Decisions

📋 Tasks

codecov bot commented Jul 3, 2023 • edited Loading

Codecov Report

aloisklink left a comment

Choose a reason for hiding this comment

sidharthv96 commented Jul 3, 2023 • edited Loading

sidharthv96 left a comment

Choose a reason for hiding this comment

jgreywolf commented Jul 3, 2023

aloisklink commented Jul 5, 2023 • edited Loading

jgreywolf commented Jul 11, 2023

netlify bot commented Aug 26, 2023 • edited Loading

✅ Deploy Preview for mermaid-js ready!

jgreywolf commented Aug 26, 2023

aloisklink left a comment

Choose a reason for hiding this comment

sidharthv96 commented Aug 28, 2023

sidharthv96 commented Aug 28, 2023 • edited Loading

jgreywolf commented Aug 28, 2023

sidharthv96 commented Aug 29, 2023 • edited Loading

sidharthv96 commented Aug 29, 2023

sidharthv96 Sep 6, 2023

Choose a reason for hiding this comment

jgreywolf commented Sep 8, 2023

jgreywolf commented Jul 3, 2023 •

edited by sidharthv96

Loading

codecov bot commented Jul 3, 2023 •

edited

Loading

sidharthv96 commented Jul 3, 2023 •

edited

Loading

aloisklink commented Jul 5, 2023 •

edited

Loading

netlify bot commented Aug 26, 2023 •

edited

Loading

sidharthv96 commented Aug 28, 2023 •

edited

Loading

sidharthv96 commented Aug 29, 2023 •

edited

Loading