-
Notifications
You must be signed in to change notification settings - Fork 75
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
Integrate mermaid.js to render graphs in markdown #375
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA. In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR. CLA has not been signed by users: @matthieucan |
@@ -45,6 +45,7 @@ | |||
"webpack-inline-svg-plugin": "^1.1.4" | |||
}, | |||
"dependencies": { | |||
"mermaid": "^9.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest version 10.0.0 has issues with Webpack: mermaid-js/mermaid#4094
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That issue was for v9.4.0.
Can you share what issue you're having for 10.0.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I might have overlooked this.
With 10.0.0, I'm getting:
ERROR in ./node_modules/mermaid/dist/mermaid.core.mjs 17:2-5
Can't import the named export 'l' from non EcmaScript module (only default export is available)
@ ./src/app/services/code_service.js
@ ./src/app/main/index.js
@ ./src/app/index.run.js
(and many more import errors following)
This might be an issue in how webpack is configured, my knowledge is limited in this area. But simply changing the version to 9.4.0 does the trick 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had another look at this, mermaid 10 requires newer versions of webpack and friends, so I propose to keep this as an improvement for later
Dear maintainers, could you let me know if you'd like to have this merged, of if it doesn't fit your plans? |
Hello. Is it still a plan to incorporate mermaid diagram into dbt and if yes how far along are your in the process of this? |
I unfortunately didn't get an answer from the maintainers about this. If you need the feature though, it works well in our fork: https://github.com/PicnicSupermarket/dbt-docs/blob/fork/FORK.md#changelog |
hey @matthieucan -- i could be reading this wrong, but it looks like they still need you to sign the CLA (the messages are confusing implying this is done), and have a change-log entry? |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
Hi @dtger, I had signed the CLA, looks like the bot is happy now :) And just pushed a changelog entry! |
Excellent, thanks for your efforts man! Looks like we're just waiting for maintainers to notice and review this, before it can be merged. I'm not sure there's a way to "summon" maintainer to review, @dbeatty10 long shot is that something you could help with perhaps? |
This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days. |
Let's keep it open, it's still relevant AFAIK. |
@matthieucan Maybe there's a chance they will look at it if all 5 checks are passing? |
You think so? To be honest I don't feel like doing all that if the PR is rejected. But feel free to open an issue for the docs, and I'd be happy to add the changelog entry |
This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days. |
Keep it open ;) |
resolves #338
Description
Hello,
This PR integrates mermaid.js into dbt docs.
The idea is to render markdown code blocks using the
mermaid
language into their corresponding visual representation.For example, the following code block:
will render as:
This enhances the documentation capabilities, and opens the door to lots of interesting things, such as ER diagrams.
The current implementation is very simple, and it mirrors what is currently being done to highlight other code blocks which originate from markdown doc blocks:
Prism.highlightAll
is called to render everything it finds (this is a different process than what's being done to render the "Code" part of a model). After Prism has run, Mermaid can similarly run too.I'm of course happy to iterate over it.
Checklist
changie new
to create a changelog entry