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: add support for mermaid diagrams in markdown #715

Closed
wants to merge 2 commits into from

Conversation

j-h-a
Copy link

@j-h-a j-h-a commented Apr 12, 2023

Description

This change adds support to include mermaid diagrams directly in any markdown.

Related issue(s)

Fixes #712

@j-h-a j-h-a changed the title feat: Add support for mermaid diagrams in markdown feat: add support for mermaid diagrams in markdown Apr 12, 2023
@j-h-a
Copy link
Author

j-h-a commented Apr 20, 2023

I'm not a React or JS/TS developer so I'm not sure about best practices or good patterns to use here - I'd appreciate some feedback on the following things, so that I can make this better before taking it out of draft:

  1. Is there a better approach/pattern that using a global boolean to track if mermaid.js is included yet?
  2. Testing: I don't know how to do testing in React, can anyone help me or point me in the right direction. I'd suggest the following tests:
  • The mermaid <script> is included exactly once when one ```mermaid code-section is present in the markdown.
  • The mermaid <script> is included exactly once when multiple ```mermaid code-sections are present in the markdown.
  • The mermaid <script> is not included when no ```mermaid code-sections are present in the markdown.
  • When language (the bit after ```) is mermaid, then a <pre class="mermaid"> section is inserted instead of a <code> section.
  • When language is not mermaid then it uses a <code> section instead.

@derberg
Copy link
Member

derberg commented Apr 24, 2023

@j-h-a thanks for that PR 🍺 and sorry again for huge delay 😞

Just to clarify, we are not using makrdown-it in react component but marked. Just keep in mind for future that in the main library we can only use libraries that are explicitly defined as dependencies -> https://github.com/asyncapi/asyncapi-react/blob/next/library/package.json#L70-L79. I think you just looked into master first.

It is the same with mermaid. I assume you've hacked it this way with global variable and dynamic script addition to not add to much more KB to bundled JS files. The problem is that this way we do not control mermaid version through package.json and will not get any potential automated dependabot updates of patch security fixes.

  • please just install mermaid package in library folder
  • just import it as any other extenral library inside marked.ts
  • adjust your code a little bit to match new setup
  • instead of making sure the script is there already, rather make sure mermaid was already initialized once and do not need to be initialized.
  • you have some code for inspiration in https://github.com/asyncapi/website/blob/master/components/MDX.js

After refactor you can check the size of bundled JS file and I bet it did not increase too much.


Regarding tests, we have simple tests and in case of your change just add a test case to https://github.com/asyncapi/asyncapi-react/blob/next/library/src/helpers/__tests__/marked.test.ts called should render mermaid diagram.


Some magic in tailwind configuration has to probably be introduced as by default the background of the diagram should be white. I think now default background for pre tags is applied


Also would make sense to mention somewhere in the readme support for mermaid

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Just left some suggestions 🙂 They are rough code suggestion, you might need to alter them slightly and add some dependencies.

markdownRenderer.code = (code, language, isEscaped) => {
if (language === 'mermaid') {
initializeMermaidOnce();
return '<pre class="mermaid">' + code + '</pre>';
Copy link
Member

Choose a reason for hiding this comment

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

Dont you need to return the rendered mermaid diagram here? i.e. add this from the website:

let currentId = 0;
const uuid = () => `mermaid-${(currentId++).toString()}`;
function MermaidDiagram({ graph }) {
  const [svg, setSvg] = useState(null);

  useLayoutEffect(() => {
    if (!graph) {
      return;
    }

    try {
      mermaid.mermaidAPI.render(uuid(), graph, renderedSvg => setSvg(renderedSvg));
    } catch (e) {
      setHtml(null)
      console.error(e);
    }
  }, [graph]);

  return svg ? <div dangerouslySetInnerHTML={{ __html: svg }} /> : null;
}

And then do the following:

Suggested change
return '<pre class="mermaid">' + code + '</pre>';
return <MermaidDiagram graph={code} />;

const markdownRenderer = new marked.Renderer();
markdownRenderer.code = (code, language, isEscaped) => {
if (language === 'mermaid') {
initializeMermaidOnce();
Copy link
Member

Choose a reason for hiding this comment

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

Remove this.

@@ -15,7 +16,7 @@ hljs.registerLanguage('yaml', yaml);
import bash from 'highlight.js/lib/languages/bash';
hljs.registerLanguage('bash', bash);

Copy link
Member

@jonaslagoni jonaslagoni May 1, 2023

Choose a reason for hiding this comment

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

Globally inline everything, just as the website does it:

Suggested change
let mermaidInitialized = false;
initializeMermaid();
function initializeMermaid() {
if (mermaidInitialized) {
return;
}
mermaidInitialized = true;
mermaid.initialize({
startOnLoad: true,
theme: "base",
securityLevel: "loose",
themeCSS: ``,
themeVariables: {
primaryColor: "#EDFAFF",
primaryBorderColor: "#47BCEE",
secondaryColor: "#F4EFFC",
secondaryBorderColor: "#875AE2",
fontFamily: "Inter, sans-serif",
fontSize: "18px",
primaryTextColor: "#242929",
tertiaryColor: "#F7F9FA",
tertiaryBorderColor: "#BFC6C7"
}
});
}

This will most likely remove the error you have, about re-exporting mermaid from the initializeMermaidOnce.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Aug 30, 2023
@derberg
Copy link
Member

derberg commented Sep 28, 2023

next will soon be deleted. It is already merged into master and 1.0 is now available.

no more long living release branches!

sorry for any trouble with your PR 😞 but we really had to finally release this v1 and get rid of release branch

I close this PR as I think it is better for you to recreate a new fresh PR targeting master. Feel free to drop a comment if you disagree and I will reopen ☮️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants