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 option for custom theme #24

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@kellerj

kellerj commented Nov 22, 2017

OK - per the discussion on issue #18 , here is the PR which adds a new -T / --customTheme option to the CLI which will fully replace the contents of the style element in the SVG tag. I also added a slightly modified version of the base mermaid.css file so that the effect of new option can be seen.

Ideally, the base mermaid config would allow this, but I checked the code and it does not support using anything but the built-in themes via the JSON config file.

@@ -76,5 +75,6 @@ Usage: mmdc [options]
-b, --backgroundColor [backgroundColor] Background color. Example: transparent, red, '#F0F0F0'. Optional. Default: white
-c, --configFile [config] JSON configuration file for mermaid. Optional
-C, --cssFile [cssFile] CSS alternate file for mermaid. Optional
-T, --customTheme <customThemeCssFile> CSS file replacing CSS injected into SVG container. Optional

This comment has been minimized.

@agusmba

agusmba Nov 22, 2017

Contributor

Not saying <...> is wrong but maybe [customThemeCssFile] for consistency?

This comment has been minimized.

@kellerj

kellerj Nov 24, 2017

Yea - I was weighing consistency over correctness here. If you use the <...> version, then commander stops the run and reports the missing option. My inclination was to change all the others to <...> but that seemed presumptuous to do as part of my change.

That said, I'm fine with switching to the [...] version if you like. I'd just want to make sure that the application handles a missing file name gracefully.

This comment has been minimized.

@kellerj

kellerj Nov 24, 2017

FYI - I just tested using one of the existing options. As it stands, if you just specify one of the options which expects a file but don't provide one, you get this.

$  node index.js -C -i test/flowchart.mmd

CSS file "true" doesn't exist

This comment has been minimized.

@agusmba

agusmba Nov 24, 2017

Contributor

I'm with you regarding changing all, @tylerlong is the developer I just proposed a couple improvements like you did.

This comment has been minimized.

@kellerj

kellerj Nov 26, 2017

OK - I've included that in this PR now as well.

This comment has been minimized.

@tylerlong

tylerlong Feb 5, 2018

Contributor

I don't get the point. If you specify -C, you should specify a CSS file such as -C ./test/index.css.

This comment has been minimized.

@tylerlong

tylerlong Feb 5, 2018

Contributor

I don't have any issue with the current commander code. You can omit any options except those marked as required. So I probably won't change this part.

@agusmba

This comment has been minimized.

Contributor

agusmba commented Nov 22, 2017

I like it! This will allow me to work-around knsv/mermaid#587
Thanks @kellerj !
Hopefully @tylerlong will be able to take a look soon.

@tylerlong

This comment has been minimized.

Contributor

tylerlong commented Feb 5, 2018

For the custom theme thing. I prefer to do it in mermaid instead of doing it in a hacky way in this project. I will leave this PR open for now.

Thank you very much for your PR!

@tylerlong

This comment has been minimized.

Contributor

tylerlong commented Mar 15, 2018

This feature is supported in mermaid. Usage:

mmdc -i input.mmd -o output.png -c config.json

https://github.com/mermaidjs/mermaid.cli/blob/master/test/config.json#L14

"theme": "forest",
"themeCSS": ".node rect { fill: red; }"

So the themeCSS could override the CSS in forest theme.

Or you could:

"theme": null,
"themeCSS": "<CSS for your theme>"

to disable pre-defined mermaid theme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment