-
-
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
feat(1624): first draft, prototype of context map grammar and diagram #5353
base: develop
Are you sure you want to change the base?
feat(1624): first draft, prototype of context map grammar and diagram #5353
Conversation
❌ Deploy Preview for mermaid-js failed.
|
As mentioned in the todo, my current focus is on addressing some key pain points:
These issues are readily apparent in the demo I'm completely open to suggestions and advice for enhancing the graph implementation. Please feel free to share here any thoughts or ideas you have in mind! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5353 +/- ##
===========================================
+ Coverage 43.37% 43.41% +0.04%
===========================================
Files 23 23
Lines 5130 5134 +4
Branches 23 23
===========================================
+ Hits 2225 2229 +4
Misses 2904 2904
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
08ac63b
to
e002223
Compare
e002223
to
e5e3415
Compare
Can you please refer to these PRs and use langium instead of jison? |
Hi! @sidharthv96, thanks a lot for the heads up! I totally missed that earlier. edit: is it acceptable for me to target the 'next' branch at this point? |
Currently, the documentation is kind of outdated, so it may not be relevant in some aspects, especially when creating new diagrams with Langium.
You don't need to create a new package; just create new folders with the name of the diagram in:
Then please refer to the listed PRs above to know what other files should be modified or added.
Yes, the |
@@ -166,6 +166,7 @@ export interface MermaidConfig { | |||
xyChart?: XYChartConfig; | |||
requirement?: RequirementDiagramConfig; | |||
mindmap?: MindmapDiagramConfig; | |||
contextMap?: MiniContextMapLanguageDiagramConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why using Mini
prefix since everywhere in the code it is simply ContextMap?
Why using Language
part as well?
Keep it uniform
const id = 'contextMap'; | ||
|
||
const detector: DiagramDetector = (txt) => { | ||
return /^\s*ContextMap/.test(txt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to standardize diagram definitions, so please use lower case, and also add -beta
suffix to it, that would imply that the syntax in the nearest future could have breaking changes
"/*" { this.pushState("comment"); } | ||
<comment>"*/" { this.popState(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments in mermaid utilize different syntax %%
for one line, and as far as I know does not support multi-line comments (which is supported in that case, and that is nice), but it may have different syntax for this
\s+ { /* skip */ } | ||
"/*" { this.pushState("comment"); } | ||
<comment>"*/" { this.popState(); } | ||
<comment>[^"*"]|[^"/"] { /* skip */ } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood this right
[]
is a symbol class so you do not need to wrap values in quotes here, by adding"
you add the symbol double quote to the symbol class as well. If you can escape them with\
if needed. So your class excludes"
(twice) and*
from grammar, and second one excludes"
(twice) and/
, it should be
^[\*\/]
- Does it mean that we prohibit * and / from comments? What if I would like to make such a comment /*******/ ?
| EOF { return yy.getGraph() } | ||
| e EOF { return yy.getGraph() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use getGraph() here? Everything you put in db (yy
variable) during the parsing is already there
e | ||
: w | ||
| e w |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you be use more descriptive names for non-terminals? These are ok from grammars perspective, but quite unreadable
Hi @nirname Thank you for the feedback! I'll update the naming convention. Regarding Jison, I'm currently working on the ContextMap Langium parser here, so the Jison version will be changed. PS: yep, I initially called it 'mini-context-map' because it might not fully implement the language reference. However, I later removed the prefix as it didn't seem relevant. |
📑 Summary
Hi Mermaid Guys, this PR addresses the diagram discussed in this GitHub issue: 1624.
You can find a demo here: https://angelochecked.github.io/mermaid/
I've started implementing something, but it's in its early and raw stages. If anyone has a better prototype, please reach out to me (writing here) so I can contribute to it.
I've opened this draft pull request primarily to gather feedback from those more skilled than me in drawing 2D elements. I'll be working on this in my spare time, and I'm unsure when it will reach production grade. However, I'm very open to suggestions. If anyone wants to collaborate on the implementation of this diagram, please let me know.
I'm experimenting with rendering using plain d3 in this orphan branch: contextMapDiagramGluedPrototype branch.
Mermaid Devs
: I hope this draft PR doesn't create noise. If it does, I can close it and use the related issues to seek feedback.Additionally, feel free to share/post here any insights you believe could enhance both this contribution and the diagram implementation. Your input is greatly appreciated in beautifying and refining the the diagram implementation :).
I'll continue to provide updates here on the current state of the implementation.
Resolves #1624
📏 Design Decisions
I'm starting to experiment with the prototype design of this diagram using plain d3 in the simplest way possible. My decisions might be subjective, as I'm not particularly skilled in this domain of drawing d3 elements, but I'm learning along the way. I'm completely open to suggestions and improvements in the approach.
📋 Tasks
TODO:
develop
branchBig Picture
MERMAID_RELEASE_VERSION
is used for all new features.open to suggestions
Parsing
contextMap
scopecontains
prefix keywordscontains
keywordopen to suggestions
Diagram Rendering
open to suggestions