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

feat: update mermaid to 10.3.0 #603

Merged
merged 7 commits into from
Aug 5, 2023
Merged

feat: update mermaid to 10.3.0 #603

merged 7 commits into from
Aug 5, 2023

Conversation

nexeck
Copy link

@nexeck nexeck commented Jul 28, 2023

fixes: #601

@nexeck
Copy link
Author

nexeck commented Jul 28, 2023

Needs more work, sankey isn't working.

@McShelby
Copy link
Owner

Please add a sankey example taken from the Mermaid docs into the Themes Mermaid page.

@nexeck
Copy link
Author

nexeck commented Jul 28, 2023

MermaidJS is deprecating the support for directives for all new stuff.
mermaid-js/mermaid#4630 (comment)

@McShelby
Copy link
Owner

Well. Mermaid becomes a mess in my opinion but my impression might come from a lack of communication.

Anyways I will not add workarounds nor a half working Mermaid version to the release until mermaid-js/mermaid#4630 is closed and a clear alternative is announced.

@McShelby McShelby added task Maintainence work blocked Depends on other issue to be fixed first mermaid This is a topic related to Mermaid itself but not the theme labels Jul 28, 2023
@nexeck nexeck force-pushed the main branch 2 times, most recently from aa98b51 to 853da37 Compare July 28, 2023 19:20
@nexeck
Copy link
Author

nexeck commented Jul 28, 2023

Well. Mermaid becomes a mess in my opinion but my impression might come from a lack of communication.

Anyways I will not add workarounds nor a half working Mermaid version to the release until mermaid-js/mermaid#4630 is closed and a clear alternative is announced.

The alternative seems to be the yaml style config: mermaid-js/mermaid#4630 (comment)

I added a workaround which should not break compatibility with existing diagrams.

When the original diagram has no directive, its adding the default theme as yaml config, if the diagram has a directive, it adds the theme, when its missing like before this PR.

@McShelby
Copy link
Owner

McShelby commented Jul 31, 2023

The sankey graph currently breaks in this PR if the theme variant is switched from Relearn Light to Relearn Dark.

Also the generated empty JSON inside of YAML looks suspicious to me:

---
{}
---
%%{init: {"fontFamily":"monospace","sequence":{"showSequenceNumbers":true},"theme":"dark"}}%%
sequenceDiagram
    Alice->>John: Hello John, how are you?
    loop Healthcheck

@nexeck
Copy link
Author

nexeck commented Jul 31, 2023

Thanks for the feedback, i will look into this next week.

@nexeck
Copy link
Author

nexeck commented Aug 4, 2023

The sankey graph currently breaks in this PR if the theme variant is switched from Relearn Light to Relearn Dark.

Also the generated empty JSON inside of YAML looks suspicious to me:

---
{}
---
%%{init: {"fontFamily":"monospace","sequence":{"showSequenceNumbers":true},"theme":"dark"}}%%
sequenceDiagram
    Alice->>John: Hello John, how are you?
    loop Healthcheck

Both should be fixed now. And now i understand, what you mean with mermaidJS is a mess.

McShelby added a commit that referenced this pull request Aug 5, 2023
McShelby added a commit that referenced this pull request Aug 5, 2023
@McShelby
Copy link
Owner

McShelby commented Aug 5, 2023

Thanks for working on this.

It still did not work, because the theme setting will always be written to the dir (directive) object which does not work with the sankey graph. I've fixed this and some minor stuff.

Could you confirm that it works for your test cases.

McShelby added a commit that referenced this pull request Aug 5, 2023
@McShelby McShelby merged commit 66c8fca into McShelby:main Aug 5, 2023
1 check passed
@McShelby McShelby removed the task Maintainence work label Aug 5, 2023
@McShelby McShelby added feature New feature or request and removed blocked Depends on other issue to be fixed first labels Aug 5, 2023
@McShelby McShelby added this to the 5.19.0 milestone Aug 5, 2023
@McShelby McShelby self-assigned this Aug 5, 2023
@nexeck
Copy link
Author

nexeck commented Aug 5, 2023

Thanks for working on this.

It still did not work, because the theme setting will always be written to the dir (directive) object which does not work with the sankey graph. I've fixed this and some minor stuff.

Could you confirm that it works for your test cases.

Works for me, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request mermaid This is a topic related to Mermaid itself but not the theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mermaid: update to 10.3.0
2 participants