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

Add support for Mermaid configuration options, version and style #5

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

cmidgley
Copy link

This pull has the following features:

  • Added a new mermaid section to conf.json to allow all Mermaid configuration options to be used
  • Added new version option to specify the Mermaid version to use, defaulting to latest Mermaid when not specified
  • Added new style option to set CSS style options on the surrounding <div>
  • Updated README to document the above and include some helpful links to Mermaid documentation

As an aside, the prior Mermaid version used had a sizing problem with classDiagram (massive rendering), but the latest (8.5.0) messes up graph (very tall boxes around boxes). If you set the flowchart option "htmlLabels": false this problem goes away. This is shown in the example configuration in the README, but hopefully this problem will be fixed in a future Mermaid release.

readme.md Outdated
}
```

This package also some new properties to the mermaid configuration:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: missing a verb / grammar. "also supports ... "

Copy link
Collaborator

@patrickdbakke patrickdbakke left a comment

Choose a reason for hiding this comment

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

Nit: var => const or let: repeated a few times :D

@cmidgley
Copy link
Author

I made the changes you recommended and also merged your refactored code. I attempted to match your style for the changes, but wasn't totally clear on the best way to handle the fact that you were using templates that won't work (at least not as global constants) with these changes. My thought was to still use the template syntax (${xxx}) but use a string and .replace to make the change. If this isn't what you think appropriate just let me know.

Thanks for being so responsive!

@cmidgley cmidgley requested a review from patrickdbakke April 22, 2020 22:28
@cmidgley
Copy link
Author

I added one more feature called disableScript which tells it to not emit the code to load or initialize Mermaid. It still does the <div> tag. This is so that you can use this when including a template that already has Mermaid code. Turns out I needed this as my JSDoc tutorial files also needed Mermaid, so I placed the Mermaid init/startup script in my template, thereby requiring this feature.

@patrickdbakke
Copy link
Collaborator

patrickdbakke commented Apr 23, 2020

@cmidgley 👍 thanks for making the changes. Since it's such a small project, I'm not sweating the linting details. What you got is great.

I'm working to get someone with our npm package ownership to release these changes. :D

@@ -1,6 +1,6 @@
{
"name": "jsdoc-mermaid",
"version": "1.0.0",
"version": "1.1.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

<3

@cmidgley
Copy link
Author

Hi - I realized that JSDoc has a serious limitation in how @tags are managed, in that you can't retain the context of the tag. As I'm sure you know, this means that all the Mermaid diagrams end up at the bottom of the comments, rather than being inline where they are referenced. I have decided to take a different approach, where I use markdown instead with the mermaid language selector, and then use a tiny javascript client-side method that adds a mermaid class to the parent tag and this works great, with inline context. This means I will no longer be using this module. Feel free to accept or reject whatever of these changes you want.

Also - I found another small issue. If the mermaid section is in a classdesc section (rather than description section, such as when you use both a class and a constructor) the code doesn't work. It looks like a simple fix, but as I said, it's not the path I am taking any more.

@Hoppi164
Copy link

Hoppi164 commented Dec 7, 2023

Hi there.

I think this change is a great improvement to the package; but i'd like to suggest one modification.
We should use mermaid.run or mermaid.init to initialize mermaid after all the page has fully loaded.

This will fix several of the problems described in issue: #9
I experienced some of these issues ( unable to render erDiagrams )
I've narrowed down the issue to being a race condition of the mermaid script starting too early.
The Mermaid docs advise to use startOnLoad: false and then to mermaid.run to instruct mermaid to start AFTER the full page has loaded.

As of mermaid v10 you should

mermaid.initialize({ startOnLoad: false });
await mermaid.run();

Before mermaid v10 you should

mermaid.initialize({ startOnLoad: false });
await mermaid.init();

https://github.com/mermaid-js/mermaid/blob/develop/docs/config/usage.md#using-mermaidrun

Alternatively I could add this change in a separate PR, however it will depend on this one being merged first, as it'll involve passing data through using your MERMAID_CONFIG

*/
const MERMAID_HTML_SCRIPT = `<script src="https://unpkg.com/mermaid@${MERMAID_VERSION}/dist/mermaid.min.js"></script>`
const MERMAID_INIT_SCRIPT = '<script>mermaid.initialize(${MERMAID_CONFIG});</script>\n'
Copy link

Choose a reason for hiding this comment

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

This is the suggestion i'm making in my comment from #5

BEFORE:

const MERMAID_INIT_SCRIPT = '<script>mermaid.initialize(${MERMAID_CONFIG});</script>\n'

AFTER:

const MERMAID_INIT_SCRIPT = '<script type=module>

  mermaid.initialize(${MERMAID_CONFIG});

  // Get major version as a number
  const MAJOR_VERSION= parseInt(MERMAID_VERSION.split('.')[0])

  // Start Mermaid after page has fully loaded
  if(MERMAID_VERSION >= 10){
    await mermaid.run();
  }
  else{
    mermaid.init();
  }
</script>\n'

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cmidgley i'd think we can prolly include this change before merging. What are your thoughts?

I think we can slightly tweak it.

const MERMAID_INIT_SCRIPT = '<script type=module>
  mermaid.initialize(${MERMAID_CONFIG});

  if (typeof mermaid.run === 'function') {
    await mermaid.run();
  } else {
    mermaid.init();
  }
</script>\n'

@andresdiaz29
Copy link

Why is this not approve it yet? I really need this plugin with the latest version of Mermaid :(

@Hoppi164
Copy link

Hoppi164 commented Oct 8, 2024

Why is this not approve it yet? I really need this plugin with the latest version of Mermaid :(

The last commit on this repo was 4 years ago.

I've also tried getting this pr merged, but i don't think this package is regularly maintained anymore, unfortunately.

@andresdiaz29
Copy link

Too sad... Maybe if you create another package and publish it, you can get the reference from Mermaid official webpage! Do you wanna try? I can be a contributor! @Hoppi164

@patrickdbakke
Copy link
Collaborator

patrickdbakke commented Oct 8, 2024

Hey @Hoppi164 @andresdiaz29 .
Apologies for the delay; thanks for the recent comments.
I am no longer employed by jellyvision, so i'm not sure what level of maintenance priority this package gets internally there.

Fortunately, AFAICT, I am still a project owner. I will review the changes and approve + merge + publish a new version later tonight.

@Hoppi164
Copy link

Hoppi164 commented Oct 8, 2024

@patrickdbakke you absolute legend!

delete cloneMermaidConfig.version
delete cloneMermaidConfig.style

let MERMAID_VERSION = version;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: this var is unused.

@@ -45,17 +53,35 @@ exports.handlers = {
recoverable: true
}).tags

let style = MERMAID_CONFIG.style ? ' style="' + MERMAID_CONFIG.style + '"' : ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

@cmidgley this is not-blocking, but we should prolly escapeHTML() the style value too.
As currently built, if we wanna use a quote in the style, this would break the html string. e.g. "style": "background: url(\"this_would_break.jpeg\")"

@patrickdbakke
Copy link
Collaborator

@cmidgley @Hoppi164 nice work on the changes here. I left some feedback on a few minor points. Let me know your thoughts on these comments. I'm ready to merge and release pending your feedback on those!

@cmidgley If you're swamped, i can make a commit on your behalf with the requested changes, but it'd slightly misattribute in the git blame. I wanna make sure you get credit. LMK your thoughts!

@andresdiaz29
Copy link

Should we add an extra validation here? Just in case

if (typeof mermaid.run === 'function') {
  await mermaid.run();
} else if (typeof mermaid.init === 'function') {
  mermaid.init();
} else {
  throw new Error('Unsupported library version');
}

@Hoppi164
Copy link

Should we add an extra validation here? Just in case

Yeah that looks good to me

@patrickdbakke
Copy link
Collaborator

@cmidgley If you're swamped, i can make a commit on your behalf with the requested changes, but it'd slightly misattribute in the git blame. I wanna make sure you get credit. LMK your thoughts!

Acknowledging some folks are waiting on these changes - if i don't hear back from @cmidgley by tomorrow, i'll make the requested changes and merge.

@cmidgley
Copy link
Author

Hi everyone - I'm not using this package these days, so if somebody else wants to take the ball and run with it, then by all means go for it. Don't worry about attributions, get what you need done in the easiest way possible!

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

Successfully merging this pull request may close these issues.

4 participants