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 mermaidjs shortcode with example #20434

Merged
merged 1 commit into from
Apr 22, 2020

Conversation

jimangel
Copy link
Member

Enabling visuals for https://mermaid-js.github.io/mermaid/ via shortcode. This was discussed at the weekly docs meeting on 4/14/2020.

I've included an example markdown doc too.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 19, 2020
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Apr 19, 2020
@netlify
Copy link

netlify bot commented Apr 19, 2020

Deploy preview for kubernetes-io-master-staging ready!

Built with commit bea8e2b

https://deploy-preview-20434--kubernetes-io-master-staging.netlify.app

@jimangel
Copy link
Member Author

Related: #20293

@jimangel jimangel force-pushed the adding-mermaid-visuals branch from a8b89ce to a40e521 Compare April 19, 2020 02:36
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 19, 2020
@jimangel
Copy link
Member Author

@jimangel
Copy link
Member Author

/assign @kbhawkey @sftim @onlydole

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Nice!
(I've limited experience with Hugo, and I do like the idea of Mermaid, but would like to help make docs available to readers who don't or can't enable JavaScript).

Is there a way to use CSS to hide the Mermaid source when JavaScript isn't enabled?

At the moment it renders roughly as

Produces:

sequenceDiagram Alice ->> Bob: Hello Bob, how are you? Bob-->>John: How about you John? Bob--x Alice: I am good thanks! Bob-x John: I am good thanks! Note right of John: Bob thinks a long
long time, so long
that the text does
not fit on a row. Bob-->Alice: Checking with John... Alice->John: Yes... John, how are you?

and there's not a hint to readers that it'd look different with JavaScript enabled.

For me the ideal would be to have the website build invoke https://github.com/mermaid-js/mermaid.cli and then save the rendered output for the CDN to serve. That's likely to work with a very large audience of readers.
However if that's not feasible, is it at least possible to replace the output with a clear indication that there's a missing image? For example, a plain mid-grey rectangle might provide enough of a hint. A rectangle with “please enable JavaScript” is even more clear.

layouts/partials/css.html Outdated Show resolved Hide resolved
@jimangel jimangel force-pushed the adding-mermaid-visuals branch from a40e521 to bea8e2b Compare April 19, 2020 19:48
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 19, 2020
@jimangel
Copy link
Member Author

@sftim I've insourced the JS, commented where the origin is, and added a JavaScript error that looks like:

image

I also moved the JS implementation where we have other JS (head.html). Let me know if you think that would resolve your concerns. Thanks!

@jimangel
Copy link
Member Author

For me the ideal would be to have the website build invoke https://github.com/mermaid-js/mermaid.cli and then save the rendered output for the CDN to serve. That's likely to work with a very large audience of readers.

I completely agree that having the user compose an image would be better from a usability standpoint. I do think by having it as JavaScript enables community easy edits / updates without much effort. I started to look into Hugo web-hooks / markup approaches but couldn't find anything concrete to dive into.

That would be one alternative, if Hugo could generate the image on build based on a hook. Once again, my goal is to enable a feature without needing much support / effort to maintain.

@sftim
Copy link
Contributor

sftim commented Apr 19, 2020

@jimangel this looks like a good approach for now, with room to make things even better at some future point

@kbhawkey
Copy link
Contributor

Hello @jimangel . I quickly looked at the mermaid docs and the content here.
A few thoughts about using this script/tool in the docs (I missed your intro).
The charts look nice. Do we need dynamic generation of charts? Could we use the tool offline to generate an image?
Is the mermaid code maintained?
Mermaid appears to have its own language to express the charts. Is this easy to use?
There seem to be some questions about security in the mermaid docs.
How do you change the fonts in the diagrams or size the generated chart on the page?
I have not used the Hugo render-image template, but it may be something to look into:
https://gohugo.io/getting-started/configuration-markup#image-markdown-example

@jimangel
Copy link
Member Author

@kbhawkey before I address your questions, let me preface this by saying my primary goal was to introduce a gantt diagram for a releases page much like: https://en.wikipedia.org/wiki/Kubernetes#Release_Versions the general consensus was that it was a good idea.

Given the velocity of the releases (every quarter) my goal was to implement something similar to Wikipedia that could be changed every 12 weeks. With that, I wanted this feature to be easily modified and low effort to maintain.

More info about the releases page here: #20293

To be clear, we don't need to merge this PR and could still create an effective releases page but I think the chart is a good visual representation of support from the community.

The charts look nice. Do we need dynamic generation of charts? Could we use the tool offline to generate an image?

I believe the velocity is the concern here. Dynamic generation really lowers the bar. The information should be additional not essential.

Is the mermaid code maintained?

Looking at the source code https://github.com/mermaid-js/mermaid it appears it is actively maintained by ~140 contributors.

The code frequency appears to be healthy with the last commit ~3 days ago and the following chart indicates code velocity is also increasing:

image

Additionally, hackmd.io supports it too: https://hackmd.io/s/features#Mermaid

Mermaid appears to have its own language to express the charts. Is this easy to use?

For my use case it seems very strait forward. I think the examples are a great reference.

There seem to be some questions about security in the mermaid docs.

I'm not aware of security concerns, but that would definitely be worth addressing before merging this PR. Can you provide some additional context / links?

How do you change the fonts in the diagrams or size the generated chart on the page?

The content can be modified via CSS and there's a few issues open regarding starting points for this. If you think this is a show stopper, I would be happy to include a custom style sheet with this PR.

I have not used the Hugo render-image template, but it may be something to look into:
https://gohugo.io/getting-started/configuration-markup#image-markdown-example

I've seen this and would be 100% down to change this approach to render on build if it can be easily done. I haven't found a good example how we could trigger image builds via mermaid (or other charting software). Going back to my original comment, I want this to be easily changed and implementable without a high bar to contribute.

@zacharysarah
Copy link
Contributor

zacharysarah commented Apr 21, 2020

@jimangel This is so plush. 🤩

Does it make sense to add a link to mermaid syntax to docs/en/contribute/style-guide/? I'd be happy to take that on as a follow-up PR.

...but the responsible chair in me thinks it would be an even better good-first-issue. 👼

@jimangel
Copy link
Member Author

jimangel commented Apr 22, 2020

+1 on the good-first-issue for the style-guide @zacharysarah. I would like feedback from @kbhawkey and if we all agree; this PR will be good to approve / merge. Thanks!

@sftim
Copy link
Contributor

sftim commented Apr 22, 2020

/lgtm

Would also welcome feedback from @kbhawkey / @onlydole

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2020
@kbhawkey
Copy link
Contributor

Hi. I am in favor of including well thought out and visually appealing diagrams in the docs 💹 .
My original question is why do we want dynamic rendering of svg files?
I agree w/@sftim , ("For me the ideal would be to have the website build invoke https://github.com/mermaid-js/mermaid.cli and then save the rendered output for the CDN to serve ...").
One benefit of declaring the chart instructions in Markdown is that you can collaborate (in a pull request) on the chart design (and debug the graph instructions).
One downside of dynamic chart rendering is that the diagram/chart is not saved with the other site assets.
I think this could be a short term solution until something could be plugged into the docs build process. I wonder about an integration similar to PostCSS,
https://gohugo.io/hugo-pipes/postcss/#check-hugo-environment-from-postcssconfigjs

{{ if eq .Params.mermaid true }}
<!-- Copied from https://unpkg.com/mermaid@8.5.0/dist/mermaid.min.js -->
<script async src="{{ "js/mermaid.min.js" | relURL }}"></script>
{{ end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

@jimangel , Question:
Do you know how the field .Params.js is used?

# config.toml
js = [
  "custom-jekyll/tags",
  "script"
]

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no clue. It looks like it does the same thing twice (explicitly first then in shortcode after:

<script src="{{ "js/script.js" | relURL }}"></script>
<script src="{{ "js/custom-jekyll/tags.js" | relURL }}"></script>
{{ with .Params.js }}{{ range (split . ",") }}<script src="{{ (trim . " ") | relURL }}"></script><!-- custom js added -->

@kbhawkey
Copy link
Contributor

/lgtm

@sftim
Copy link
Contributor

sftim commented Apr 22, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3891131 into kubernetes:master Apr 22, 2020
@BenTheElder
Copy link
Member

trying to follow along, so this is still dynamically rendered right?

@jimangel
Copy link
Member Author

jimangel commented Apr 23, 2020

this is still dynamically rendered right?

Yep! However, there's a strong desire to eventually switch to generation on build; if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants