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

Adding support for mermaid directives and themeVariables #192

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

Conversation

frankieliu
Copy link

User can modify setttings.json to set mermaid directives and themeVariables.

@@ -6,17 +6,27 @@ function init() {
const darkModeTheme = configSpan?.dataset.darkModeTheme;
const lightModeTheme = configSpan?.dataset.lightModeTheme;

const directives = JSON.parse(configSpan?.dataset.directives ?? '');
Copy link
Owner

Choose a reason for hiding this comment

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

This should be wrapped in a try catch in case it fails

Example (add to `setttings.json`):
```json
{
"markdown-mermaid.directives": {
Copy link
Owner

Choose a reason for hiding this comment

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

This new setting should also be documented in the package.json

@@ -6,17 +6,27 @@ function init() {
const darkModeTheme = configSpan?.dataset.darkModeTheme;
const lightModeTheme = configSpan?.dataset.lightModeTheme;

const directives = JSON.parse(configSpan?.dataset.directives ?? '');
const directivesDebug = 'debug' in directives ? directives.debug : false;
Copy link
Owner

Choose a reason for hiding this comment

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

When would users need the debug option?

src/index.ts Outdated
const lightModeTheme = "themeVariables" in directives ?
'base' : sanitizeMermaidTheme(config.get('lightModeTheme'));

/*
Copy link
Owner

Choose a reason for hiding this comment

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

It this comment useful?

src/index.ts Outdated

// Note: single quotes needed for directives
// JSON.stringify will contain double quotes
// encodeURIcomponent could be used to be safer
Copy link
Owner

Choose a reason for hiding this comment

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

Can't the json itself still contain single quotes, such as inside strings?

src/index.ts Outdated
// Setting themeVariables will override
// darkModeTheme and lightModeTheme
// both will be set to "base"
const darkModeTheme = "themeVariables" in directives ?
Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain why themeVariables need to reset the theme?

@cooperrunyan
Copy link

@mjbvz If I submit a pr adding a try/catch around JSON.parse and documentation in package.json, will you merge it?

@mjbvz
Copy link
Owner

mjbvz commented Oct 23, 2024

Pushed fix for merge conflicts and using try catch. Still not convinced on merging this yet because:

  • It's unclear to me if it is safe to set the mermaid config from an untrusted source. It's very possible this enables script injections. However I think at least some of these options can be passed as frontmatter in the diagram too 🤷

  • Looking through the issues there doesn't seem to be a whole lot of demand for this right now

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.

3 participants