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

[Feature] Mermaid and graphviz diagrams #47

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sneakers-the-rat
Copy link
Contributor

Fix: openjournals/joss#1281
Builds on:

Add support for graphviz and mermaid diagrams using https://github.com/pandoc-ext/diagram

  • Vendored lua plugin bc no clean way to include it otherwise and i hate using submodules
  • symlinked to filters directory
  • added to default filters
  • configured to enable/diable diagram engines consistent with security recs in source repo
  • added deps to dockerfile
  • wrote examples in example/paper.md

Works in the tex, html, and jats formats

eg. PDF:

Screenshot 2024-02-28 at 9 05 36 PM Screenshot 2024-02-28 at 9 05 49 PM

Also added an example of how to include a highlighted code block because i thought it was weird we didn't have one

(Again sorry for non-modular PR, couldn't get the code to run otherwise)

@tarleb
Copy link
Collaborator

tarleb commented Apr 30, 2024

Thank you! I'm not sure yet about #45, but the other PRs look good. I'm holding off a little longer, as we should also update the pandoc version in the docker container, which is proving a bit more difficult than expected.

@sneakers-the-rat
Copy link
Contributor Author

sneakers-the-rat commented Aug 15, 2024

Thoughts on this? I rebased and the test is now failing for a reason that is totally mysterious to me.

Looks like it's not this PR: https://github.com/openjournals/inara/actions/runs/9450955436/job/26030778792

@tarleb
Copy link
Collaborator

tarleb commented Aug 16, 2024

Looking good. I still have some reservations, mostly because of the tremendous size of the mermaid-cli tool: it requires a full browser to be installed in the docker image, making the image more difficult to use for people with bad/limited internet connections.

CC @arfon, I'd be interested to hear your take on this.

As for the test failures: yes, that's just a problem with the test workflow: the login and push actions should only run on the main branch. Otherwise they'll fail due to insufficient permissions.

@sneakers-the-rat
Copy link
Contributor Author

the tremendous size of the mermaid-cli tool:

oops, ya you're right. there's gotta be a smaller tool than that, mermaid.js is like 3MB. i'll take a look

the login and push actions should only run on the main branch.

I guess that's the next fix then :) but there also seems to be a failure in the pdf build that looks unrelated? like a latex error? and it started happening in other PRs too recently

@tarleb
Copy link
Collaborator

tarleb commented Aug 16, 2024

I've opened #70 to keep track of the PR issue.

@arfon
Copy link
Member

arfon commented Aug 20, 2024

Definitely would be cool so support this so thanks for the contribution here @sneakers-the-rat! Did you manage to find a smaller Docker image to support this?

@sneakers-the-rat
Copy link
Contributor Author

now that i'm thinking about this again, i think we'll also want version pinning, especially re: mermaid. maybe we can do both things at once - i wonder if there is a way to peek at the yaml frontmatter when making the docker image? and if we did that we could see if a given doc uses a particular dependency. I think we'll want to do this generally if we want to do flexible document formatting (which i think would be a lovely goal), so let me take a look at that, unless y'all know of a way already :)

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.

Mermaid diagrams in Markdown-formatted JOSS articles
3 participants