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/4381 markdown in sequence diagram #4725

Draft
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

ibrahimWassouf
Copy link
Contributor

📑 Summary

Implements markdown in sequence diagrams

Resolves #4381 #1844 #

📏 Design Decisions

I've stuck to simply imitating what other diagrams have implemented and reused as much code as I could.

📋 Tasks

Make sure you

@ibrahimWassouf ibrahimWassouf marked this pull request as draft August 11, 2023 23:57
@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

Merging #4725 (4ebac5f) into develop (ed819e9) will increase coverage by 0.38%.
The diff coverage is 88.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4725      +/-   ##
===========================================
+ Coverage    77.05%   77.43%   +0.38%     
===========================================
  Files          146      146              
  Lines        14573    14632      +59     
  Branches       592      594       +2     
===========================================
+ Hits         11229    11331     +102     
+ Misses        3225     3183      -42     
+ Partials       119      118       -1     
Flag Coverage Δ
e2e 84.46% <98.48%> (+0.53%) ⬆️
unit 45.50% <8.33%> (-0.05%) ⬇️

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

Files Changed Coverage Δ
packages/mermaid/src/utils.ts 61.25% <33.33%> (-0.23%) ⬇️
packages/mermaid/src/diagrams/sequence/svgDraw.js 89.13% <97.82%> (+0.83%) ⬆️
...ckages/mermaid/src/diagrams/sequence/sequenceDb.js 84.84% <100.00%> (+0.47%) ⬆️
.../mermaid/src/diagrams/sequence/sequenceRenderer.ts 88.79% <100.00%> (+0.09%) ⬆️

... and 8 files with indirect coverage changes

@ibrahimWassouf
Copy link
Contributor Author

Note*: It seems that git diff goofed a bit for the svgDraw.js file. I think it's best to view it in an editor to see the real difference in the code. In summary, there were two conditional branches that were in both the drawText and drawMarkdownText functions. I simply made them independent functions and called them.

@ibrahimWassouf ibrahimWassouf force-pushed the feature/4381_markdown_in_sequence_diagram branch from 8e70fc2 to 925a419 Compare August 18, 2023 01:32
@ibrahimWassouf
Copy link
Contributor Author

Seems like I'm having issues with conflicts in pnpm.lock. All I'm trying to do is merge develop into this branch. I have never downloaded an extra dependency or changed anything so I'm not really sure how this came about.

@netlify
Copy link

netlify bot commented Aug 26, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 4ebac5f
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/64ea4b679077af000848173f
😎 Deploy Preview https://deploy-preview-4725--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.

@nirname
Copy link
Contributor

nirname commented Dec 12, 2023

@ibrahimWassouf Hi! Any updates on this one?

@ibrahimWassouf
Copy link
Contributor Author

@ibrahimWassouf Hi! Any updates on this one?

Hey! I got busy for a while and left this in this state obviously. In short, I can't think of a way to do this with the sequence diagram as it is (although I will have to check if the code changed significantly since starting). The issue is that strings are cut up into smaller, separate strings and stacked on top of each other. This means that if you had "A string with a a really long emphasis" is could be cut in two between the tags, preventing them from being properly rendered.

The biggest issue I came across was that other elements in the diagram, such as the width of the actors/participant boxes or when the message arrow is rendered, are rendered dynamically based on the size of the message. Thus, creating a singe text label did not work at nicely as I'd hoped.

It all seemed like spaghetti code to me, so I had trouble figuring it out. I started work on migrating the sequence diagram to typescript in hopes of understanding how the whole thing is rendered but didn't have the time to finish that up.

I'll give it my best shot to give this another try, but feel free to unassign me from the ticket if you want to encourage others to give it a try as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Markdown Strings to sequence diagrams
2 participants