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

Feature/4051 sequence diagram multi directional arrow #5209

Conversation

Ronid1
Copy link
Contributor

@Ronid1 Ronid1 commented Jan 19, 2024

📑 Summary

Added Bi-directional arrow support for dotted and solid lines in sequence diagram

Resolves #4051

📏 Design Decisions

Added new syntax <<->> for solid bidirectional arrow
Added new syntax <<-->> for dotted bidirectional arrow
Please approve new syntax.

Added unit tests for both arrows.

📋 Tasks

Make sure you

@github-actions github-actions bot added the Type: Enhancement New feature or request label Jan 19, 2024
Copy link

netlify bot commented Jan 19, 2024

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 1a199d6
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/66746655d903000008c7e8e6
😎 Deploy Preview https://deploy-preview-5209--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Project coverage is 5.73%. Comparing base (3af4020) to head (1a199d6).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #5209      +/-   ##
==========================================
- Coverage     5.73%   5.73%   -0.01%     
==========================================
  Files          278     277       -1     
  Lines        41999   42013      +14     
  Branches       490     515      +25     
==========================================
  Hits          2409    2409              
- Misses       39590   39604      +14     
Flag Coverage Δ
unit 5.73% <0.00%> (-0.01%) ⬇️

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

Files Coverage Δ
packages/mermaid/src/diagrams/sequence/svgDraw.js 0.00% <0.00%> (ø)
...ckages/mermaid/src/diagrams/sequence/sequenceDb.ts 0.00% <0.00%> (ø)
.../mermaid/src/diagrams/sequence/sequenceRenderer.ts 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

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.

LGTM! Nice work :)

Please add corresponding rendering tests as well in cypress/integration/rendering/sequencediagram.spec.js

@Ronid1
Copy link
Contributor Author

Ronid1 commented Jan 19, 2024

@sidharthv96 I added e2e test and modified the syntax, since the < char was parsed as an HTML open tag.
Updated new syntax ->*> for solid bidirectional arrow
Updated new syntax -->*> for dotted bidirectional arrow

Should I add documentation in a separate PR?

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.

Technically, the changes are perfect.
The only concern is the proposed syntax. It's unfortunate that we can't use <.

Is -*> and --*> better, so that we don't go into 5 character arrows?

cypress/integration/rendering/sequencediagram.spec.js Outdated Show resolved Hide resolved
@Ronid1
Copy link
Contributor Author

Ronid1 commented Jan 29, 2024

Agree that -*> and --*> are better. updated.

@sidharthv96 sidharthv96 requested a review from a team January 31, 2024 11:59
@sidharthv96
Copy link
Member

https://mermaid.js.org/syntax/flowchart.html#multi-directional-arrows

We are using <--> for multidimensional arrows. And it'll be supported in markdown usecases, and live editor.
The only problem is when we write an HTML page directly no?

@Ronid1
Copy link
Contributor Author

Ronid1 commented Feb 8, 2024

@sidharthv96 Yes, from what I can see the ->< syntax is only problematic when written in HTML since the opening < tag is left unclosed. I am actually able to use <-> <--> syntax for sequence both with Mermaid engine and HTML if that is preferred, though not sure it aligns with the current syntax that uses a double arrowhead sign >> and <<->> <<-->> are a bit excessive. Another option that works in both cases is -<> --<>

@Ronid1
Copy link
Contributor Author

Ronid1 commented Feb 29, 2024

@sidharthv96 would love to make progress with this PR, could you please help me tag the correct people for the second review you requested?

@sidharthv96
Copy link
Member

<<->> and <<-->> are the clear choice from a uniformity and intutiveness perspective, and feels like a worthy tradeoff against line length. If someone is editing a diagram with a -->>, and wants to add a left arrow, the natural thing to try is <<-->>, unless they consult the doc and have the syntax in memory.

I know we've gone back and forth on this for a while, but this seems like the best option for our users. Initially I thought we could never use <, but as that's not the case, I'm good with this option.

@mermaid-js/atlantians thoughts?

@knsv
Copy link
Collaborator

knsv commented Mar 25, 2024

@Ronid1 Thanks for adding this feature. One of the challenges for Mermaid is to keep the syntaxes coherent between the different diagrams. The proposed syntax offers better readability and stays pretty consistent with the flowchart syntax for multi-directional arrows.

@sidharthv96
Copy link
Member

@Ronid1 can you please resolve the conflicts?

@Ronid1 Ronid1 force-pushed the feature/4051_sequence_diagram-multi-directional-arrow branch from 08e4142 to e852156 Compare April 24, 2024 03:39
@Ronid1
Copy link
Contributor Author

Ronid1 commented Apr 24, 2024

@sidharthv96 should be good to go now

@sidharthv96
Copy link
Member

Just noticed that the arrow is overlapping the box by a few pixels? Can you fix that?

Proper position
image

Incorrect
image

@Ronid1
Copy link
Contributor Author

Ronid1 commented Apr 24, 2024

@sidharthv96 arrow pointer position fixed

@Ronid1
Copy link
Contributor Author

Ronid1 commented May 13, 2024

@sidharthv96 bumping

@sidharthv96
Copy link
Member

sidharthv96 commented May 23, 2024

Found this issue while testing the suggested fix

image

The arrow doesn't show up when autoNumbering is on.

image

And the circle is misaligned, when compared to uni directional arrows.

The same issue is present in the current implementation as well, although in a slightly different manner.
image


I don't want to block this PR based on this issue, unless you want to fix it before merge.
To reiterate, there are 2 issues when auto numbering is on, the arrowhead not being visible, and the alignment of the circle being off.

@Ronid1
Copy link
Contributor Author

Ronid1 commented Jun 4, 2024

I think the auto-numbering should be solved as a separate issue. Seems to me that the approach to fixing that has to do with moving the position of the number, and not with the arrowhead. Happy to open an issue and work on it.

…feature/4051_sequence_diagram-multi-directional-arrow
Copy link

argos-ci bot commented Jun 20, 2024

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) 👍 Changes approved 1 added Jun 20, 2024, 5:38 PM

@Ronid1
Copy link
Contributor Author

Ronid1 commented Jun 20, 2024

bumping @sidharthv96

@sidharthv96 sidharthv96 added this pull request to the merge queue Jun 20, 2024
Merged via the queue into mermaid-js:develop with commit cda41a1 Jun 20, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Multi directional arrow support in sequence diagrams
4 participants