Skip to content

fix: clean comments in text in getDiagramFromText API so flowchart works well#5076

Merged
sidharthv96 merged 9 commits intomermaid-js:developfrom
ad1992:aakansha/bug/5075-fix-getDiagramFromText-api
Nov 28, 2023
Merged

fix: clean comments in text in getDiagramFromText API so flowchart works well#5076
sidharthv96 merged 9 commits intomermaid-js:developfrom
ad1992:aakansha/bug/5075-fix-getDiagramFromText-api

Conversation

@ad1992
Copy link
Copy Markdown
Contributor

@ad1992 ad1992 commented Nov 27, 2023

Resolves #5075

📑 Summary

This bug was introduced in v10.5.0 in #4759 and since the comments are not being cleaned when using the getDiagramFromText API, hence the flowchart with comments breaks.

📏 Design Decisions

I am calling the API cleanupComments in Diagram constructor to make sure the comments are always removed before creation and this works fine.

📋 Tasks

Make sure you

@netlify
Copy link
Copy Markdown

netlify Bot commented Nov 27, 2023

Deploy Preview for mermaid-js ready!

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

@github-actions github-actions Bot added the Type: Bug / Error Something isn't working or is incorrect label Nov 27, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 27, 2023

Codecov Report

Merging #5076 (4ed7b2b) into develop (25e9bb3) will increase coverage by 36.38%.
The diff coverage is 66.66%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #5076       +/-   ##
============================================
+ Coverage    42.91%   79.29%   +36.38%     
============================================
  Files           23      165      +142     
  Lines         5029    13863     +8834     
  Branches        21      704      +683     
============================================
+ Hits          2158    10993     +8835     
+ Misses        2870     2720      -150     
- Partials         1      150      +149     
Flag Coverage Δ
e2e 85.19% <66.66%> (?)
unit 42.91% <ø> (ø)

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

Files Coverage Δ
packages/mermaid/src/mermaidAPI.ts 77.96% <66.66%> (ø)

... and 159 files with indirect coverage changes

@ad1992
Copy link
Copy Markdown
Contributor Author

ad1992 commented Nov 27, 2023

@sidharthv96 please let me know if you have concerns with the changes

Comment thread demos/flowchart.html Outdated
Comment thread packages/mermaid/src/Diagram.ts Outdated
@sidharthv96
Copy link
Copy Markdown
Member

Btw, if you already have a PR ready, you can raise it directly without creating an issue first. :)

Comment thread packages/mermaid/src/diagram.spec.ts Outdated
@ad1992 ad1992 requested a review from sidharthv96 November 28, 2023 03:41
@sidharthv96 sidharthv96 added this to the v10 milestone Nov 28, 2023
@ad1992
Copy link
Copy Markdown
Contributor Author

ad1992 commented Nov 28, 2023

Looks like the checks have hanged as the status isn't showing up so I will try pushing a dummy commit

Comment thread packages/mermaid/src/mermaidAPI.spec.ts Outdated
auto-merge was automatically disabled November 28, 2023 06:35

Head branch was pushed to by a user without write access

@ad1992
Copy link
Copy Markdown
Contributor Author

ad1992 commented Nov 28, 2023

@sidharthv96 you can merge this PR, I had to push a commit to trigger GH checks hence auto merging was disabled

@sidharthv96 sidharthv96 added this pull request to the merge queue Nov 28, 2023
Merged via the queue into mermaid-js:develop with commit 4499926 Nov 28, 2023
@ad1992 ad1992 deleted the aakansha/bug/5075-fix-getDiagramFromText-api branch July 8, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug / Error Something isn't working or is incorrect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flowchart comments broken v10.5.0 when using getDiagramFromText API

2 participants