-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
New Diagram: Architecture #5452
base: develop
Are you sure you want to change the base?
New Diagram: Architecture #5452
Conversation
…s/mermaid into 5367/architecture-diagram
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5452 +/- ##
==========================================
- Coverage 5.86% 5.20% -0.67%
==========================================
Files 274 298 +24
Lines 41087 46514 +5427
Branches 488 537 +49
==========================================
+ Hits 2408 2419 +11
- Misses 38679 44095 +5416
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@sidharthv96 Continuing our conversation from the issue, I commented on SVG loading and my thought process behind how the layout is handled. Let me know what your thoughts are. Additionally, an example of what forked edges would look like once implemented: Regarding multiple edges going into one side of a service, it's possible but I'll need to spend some time figuring out the cleanest way to implement that functionality. Ultimately I was planning on using forked edges to handle that scenario but after looking at your example the arrows can't be as neatly represented the same way through forked edges. |
Just to share some opinions
Imho, yes. Architecture diagrams tend to get very large and sometimes some manual positioning would be extremely helpful
I would include them and offer the possibility to reference external images. The reason for this is that otherwise we will run into CORS issues e.g. in the GitHub integration as external SVGs cannot be loaded to prevent XSS. |
@sidharthv96 When you have the chance, could we get the review process started for this PR? I'm happy with the results as it stands and getting some feedback would be great! I've recently added documentation for using icons and once I know the implementation details won't change I'll update the docs with the available icons (ideally in an automated fashion akin to how the contributors are added). |
* develop: [autofix.ci] apply automated fixes [autofix.ci] apply automated fixes docs: Test autofix.ci chore: Remove update step from lint.yml Add autofix.ci fix: double space in wrapped sequence diagram messages chore: update browsers list chore(deps): update eslint chore: move abs below return check fix: Handle negative numbers in `formatBytes` chore: Use single quotes fix: emphasis => em fix: Handle spaces after newline test: Change emphasis to em chore: Fix emphasis type feat: Use marked instead of mdast-util-from-markdown Add Madness to integrations-community.md
The latest updates on your projects. Learn more about Argos notifications ↗︎ Waiting for the first build to start… |
Looking at the examples it's not clear to me how to give the edges arrow ends? Can we do this similar to flow charts? |
You can add For example:
|
Not being a mermaid or architecture native, consistency would be a plus for me. Worth noting that the |
📑 Summary
Architecture diagrams allows users to show relations between services
Resolves #5367
📏 Design Decisions
Parser
Terminology
Syntax
Currently, the syntax is defined as:
service {id}({icon})[{title}] (in {group_name})?
group {id}[{title}] (in {group_id})?
{id_1} ( ( )?(L|R|T|B)--(L|R|T|B)( ) )? {id_2}
Once I transition from jison to Langium, I plan to add the following extensions:
group {id}({icon})[{title}] (in {group_id})?
{id_1} ( ( )?(L|R|T|B)-([{label}])-(L|R|T|B)( ) )? {id_2}
service {id}( ({icon}) | ("{text}") )[{title}] (in {group_name})?
Additionally, the syntax for groups (and potentially forked edges?) will be modified to better follow a declare then organize approach
Layout
Comments
As mentioned in the issue, the LRTB syntax goes against the ethos of Mermaid. Additionally, while the spatial map creates very neatly organized layouts, it prevents users from having multiple edges going out of one side. Part of the reason I began designing it this way was to avoid this diagram type from essentially being a flowchart with icons.
Going forward, there's a few options that can fix this problem:
fcose
and let it figure things out on its own.No matter which option we choose, the LRTB syntax should be extended to:
SVG Icons
SVG icons are currently created and accessed through helper functions inside of rendering-util/svgRegister.ts. A global object stores the mapping of icon names to
IconResolvers
defined asIconResolvers can easily be created through the helper function
createIcon
. This function takes the SVG code as a string along with the original resolution. It returns a CB function taking the element to inject the SVG into and optionally a different size to scale it by.A few generic icons I've created are located in rendering-utils/svg/. Additional icons can be added through the new
iconLibraries
config option. The types insvgRegister.ts
are now exported frommermaid.ts
, allowing developers to create their own icon library packages.Alternatives
From conversations in the linked issue, ZenUML already handles SVG icons on their end. Upon furthur inspection, they rely on declaring an svg module which essentially resolves the imported SVG as a string. Since the end result of both of these approaches are the same (having the SVG code as a string), personally I think it's debatable if we want to go through with this extra step. The main con I foresee is that it'll add more work for people who want to make their own icon library package as they will need to setup the declarations themselves.
On a side note, it looks like ZenUML directly added the official AWS icons to their repo here. IANAL but the terms Amazon states are:
This may mean we can make official icon packs in seperate packages, or have them be lazy loaded as needed. Ultimately I'll leave that decision up to you as that isn't my area of specialties.
Todo
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.develop
branch