Skip to content

Fix: Common handling of strings in Architecture chart #6489

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

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

thomascizeron
Copy link
Contributor

@thomascizeron thomascizeron commented Apr 16, 2025

๐Ÿ“‘ Summary

architecture-md

  • ๐Ÿ–‹๏ธ Add common string hanling in common grammar for Literal strings and Markdown strings
  • ๐Ÿ“Š Use this new string approach in Architecture Chart
  • โš—๏ธ Add a whole bunch of tests

Resolves issue #5928
Also resolves issue #6056

Also Resolves #6322 as the architecture diagram was already implementing Edge labels, now it works for literal strings and markdown strings too

๐Ÿ“ Design Decisions

  • Common String Handling
    • Add regex for strings and Labels in Common Grammar
      • Literal Strings starts with a simple quote or a double quotes 'Literal String' or "Literal String"
      • Markdown Strings can be:
        • As FlowChart: simple/double quotes with backticks '`Markdown **String**`' or "`Markdown _string_`"
        • ๐Ÿ†• just simple backticks `Markdown String`
    • Update common Value converter so that markdown strings start with a backtick `
  • Update Architecture Grammar to use the new grammar
    • ๐Ÿ’ฅ Breaking Changes (As the grammar is still in beta):
      • Group, Service and Edge Labels must be in strings in brackets ['My Label'] as it is the way it works in many other charts in mermaid
      • Icons use the same string handling and must be quoted ('my-icon')
      • Made the colon : optional in edge definition ad it is not formally needed by the grammar
  • โœจ Rendering
    • When a string starts with a backticks it is displayed as markdown, else literal
      • So no need for a boolean like labelType
      • It feels like we need a global function to render text with this new approach. createText should render literal and markdown string base on if it starts with a backtick or not โžก๏ธ Could not do it for backward compatibility
    • Uses createText function to display Markdown strings
    • Creates a new createNonFormattedText to display literal strings
    • Rename group, service, edge titles as labels in variables names to be coherent
  • โš—๏ธ Tests
    • Add parser, mermaid tests for Architecture chart
    • Fix Cypress tests and add some for Architecture chart
  • ๐Ÿ“š Update documentation

๐Ÿ“‹ Tasks

Make sure you

  • ๐Ÿ“– have read the contribution guidelines
  • ๐Ÿ’ป have added necessary unit/e2e tests.
  • ๐Ÿ““ have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features.
  • ๐Ÿฆ‹ If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Copy link

changeset-bot bot commented Apr 16, 2025

๐Ÿฆ‹ Changeset detected

Latest commit: 230ba86

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
mermaid Patch
@mermaid-js/parser Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Apr 16, 2025
Copy link

netlify bot commented Apr 16, 2025

โœ… Deploy Preview for mermaid-js ready!

Name Link
๐Ÿ”จ Latest commit 230ba86
๐Ÿ” Latest deploy log https://app.netlify.com/projects/mermaid-js/deploys/6834a612d96d560008989a7c
๐Ÿ˜Ž Deploy Preview https://deploy-preview-6489--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 project configuration.

Copy link

pkg-pr-new bot commented Apr 16, 2025

Open in StackBlitz

mermaid

npm i https://pkg.pr.new/mermaid-js/mermaid@6489

@mermaid-js/layout-elk

npm i https://pkg.pr.new/mermaid-js/mermaid/@mermaid-js/layout-elk@6489

@mermaid-js/mermaid-zenuml

npm i https://pkg.pr.new/mermaid-js/mermaid/@mermaid-js/mermaid-zenuml@6489

@mermaid-js/tiny

npm i https://pkg.pr.new/mermaid-js/mermaid/@mermaid-js/tiny@6489

@mermaid-js/parser

npm i https://pkg.pr.new/mermaid-js/mermaid/@mermaid-js/parser@6489

commit: 230ba86

Copy link

codecov bot commented Apr 16, 2025

Codecov Report

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

Project coverage is 3.87%. Comparing base (818699f) to head (230ba86).

Files with missing lines Patch % Lines
...kages/mermaid/src/diagrams/architecture/svgDraw.ts 0.00% 46 Missing โš ๏ธ
packages/mermaid/src/rendering-util/createText.ts 0.00% 24 Missing โš ๏ธ
...kages/parser/src/language/common/valueConverter.ts 0.00% 20 Missing โš ๏ธ
...ermaid/src/diagrams/architecture/architectureDb.ts 0.00% 6 Missing โš ๏ธ
.../src/diagrams/architecture/architectureRenderer.ts 0.00% 4 Missing โš ๏ธ
...mermaid/src/rendering-util/handle-markdown-text.ts 0.00% 3 Missing โš ๏ธ
...ackages/parser/src/language/architecture/module.ts 0.00% 2 Missing โš ๏ธ
...s/parser/src/language/architecture/tokenBuilder.ts 0.00% 1 Missing โš ๏ธ
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #6489      +/-   ##
==========================================
- Coverage     3.87%   3.87%   -0.01%     
==========================================
  Files          414     411       -3     
  Lines        43303   43315      +12     
  Branches       666     664       -2     
==========================================
- Hits          1679    1677       -2     
- Misses       41624   41638      +14     
Flag Coverage ฮ”
unit 3.87% <0.00%> (-0.01%) โฌ‡๏ธ

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

Files with missing lines Coverage ฮ”
...aid/src/diagrams/architecture/architectureTypes.ts 0.84% <รธ> (รธ)
...s/parser/src/language/architecture/tokenBuilder.ts 0.00% <0.00%> (รธ)
...ackages/parser/src/language/architecture/module.ts 4.54% <0.00%> (รธ)
...mermaid/src/rendering-util/handle-markdown-text.ts 1.28% <0.00%> (-0.04%) โฌ‡๏ธ
.../src/diagrams/architecture/architectureRenderer.ts 0.00% <0.00%> (รธ)
...ermaid/src/diagrams/architecture/architectureDb.ts 0.38% <0.00%> (รธ)
...kages/parser/src/language/common/valueConverter.ts 2.08% <0.00%> (+0.16%) โฌ†๏ธ
packages/mermaid/src/rendering-util/createText.ts 0.42% <0.00%> (-0.05%) โฌ‡๏ธ
...kages/mermaid/src/diagrams/architecture/svgDraw.ts 0.00% <0.00%> (รธ)

... and 1 file with indirect coverage changes

๐Ÿš€ New features to boost your workflow:
  • โ„๏ธ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • ๐Ÿ“ฆ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

argos-ci bot commented Apr 16, 2025

The latest updates on your projects. Learn more about Argos notifications โ†—๏ธŽ

Build Status Details Updated (UTC)
default (Inspect) โš ๏ธ Changes detected (Review) 17 added May 26, 2025, 5:43 PM

@thomascizeron thomascizeron force-pushed the fix/5928_commonHandlingOfStringsInArchitectureChart branch from 55bb874 to 06c01a1 Compare April 19, 2025 10:17
Copy link
Collaborator

@NicolasNewman NicolasNewman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good but I have one concern from a UX perspective:

I'm a little hesitant with making the colon optional. You're right that it's not needed from a grammar standpoint, but I'd personally prefer to go one way or the other.

Leaving it optional has the potential to add confusion by letting users write the markdown for Architecture diagrams in different flavors. I personally don't like allowing that kind of ambiguity.

I can't remember why the colon was added but if I recall it was suggested in the original PR that added this diagram to make the syntax more intuitive.

I'm not particularly attached to one way or the other but I do think we need to settle on just one.

@NicolasNewman
Copy link
Collaborator

@sidharthv96 You're thoughts on making the colon optional would be appreciated.

@sidharthv96 sidharthv96 self-assigned this Apr 20, 2025
@thomascizeron thomascizeron force-pushed the fix/5928_commonHandlingOfStringsInArchitectureChart branch 2 times, most recently from 3715f07 to 64861ac Compare April 24, 2025 17:26
@thomascizeron
Copy link
Contributor Author

Hi @sidharthv96 i've fixed the issues and used inline snaphot for testing โ˜บ๏ธ

@thomascizeron thomascizeron force-pushed the fix/5928_commonHandlingOfStringsInArchitectureChart branch 2 times, most recently from de315a3 to 30c73ed Compare May 6, 2025 19:17
@thomascizeron
Copy link
Contributor Author

Hi there, I've added a commit to simplify the common valueConverter, as the previous implementation was a bit weird and not so maintainable.

@thomascizeron thomascizeron requested a review from sidharthv96 May 13, 2025 18:52
@thomascizeron thomascizeron force-pushed the fix/5928_commonHandlingOfStringsInArchitectureChart branch from 30c73ed to 6bd644c Compare May 18, 2025 13:05
@thomascizeron thomascizeron force-pushed the fix/5928_commonHandlingOfStringsInArchitectureChart branch from 6bd644c to 230ba86 Compare May 26, 2025 17:34
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.

Text for edges in architecture diagrams
3 participants