Skip to content

Conversation

@tlylt
Copy link
Contributor

@tlylt tlylt commented Dec 10, 2022

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Closes #1079
Related to #1663 for pie-chart
Related to #984 for diagramming tool

Add an optional plugin that can be enabled to support mermaid-js integration. Done by providing a custom tag and works via Calling the JavaScript API method. This seems to be the most lightweight approach.

Anything you'd like to highlight/discuss:
From my testing, the mermaid syntax does not work in a panel, which seems to be due to how the mermaid library is loaded on the page and how it acts differently when contained in a panel.

Example (doesn't work in a panel, but works without)

<panel>
<mermaid>
graph TD;
    A-->B;
    A-->C;
    B-->D;
    C-->D;
</mermaid>
</panel>

<mermaid>
graph TD;
    A-->B;
    A-->C;
    B-->D;
    C-->D;
</mermaid>

image

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Add optional mermaid plugin

Existing MarkBind support for graphs is still limited.

Mermaid-js lets users create diagrams and visualizations using text
and code.

Let's add an optional plugin that users can enable if they choose
to create mermaid-supported diagrams.

This feature is added as an optional plugin as some diagrams
supported by mermaid overlaps with PlantUML, which we support
by default.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@tlylt tlylt changed the title [WIP] Add optional mermaid plugin for alternative diagram & chart support Add optional mermaid plugin for alternative diagram & chart support Dec 13, 2022
@tlylt tlylt marked this pull request as ready for review December 13, 2022 02:46
Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Nice work @tlylt 👍

Tried it out and it works great :)

From my testing, the mermaid syntax does not work in a panel, which seems to be due to how the mermaid library is loaded on the page and how it acts differently when contained in a panel.

Probably a separate issue but it seems like mermaid does not work in a modal as well:

<trigger for="modal:testMermaid">Open modal</trigger>.
<modal id="modal:testMermaid">
  <mermaid>
    graph TD;
        A-->B;
        A-->C;
        B-->D;
        C-->D;
  </mermaid>
</modal>

If the issue is on mermaid's side, then perhaps we can add a note to the user guide to mention this limitation.

Other than that, it looks good in general 👍

@tlylt
Copy link
Contributor Author

tlylt commented Dec 23, 2022

Probably a separate issue but it seems like mermaid does not work in a modal as well:

Thanks for reviewing @jonahtanjz! I think I tested include and box and both are working, but panel and modal indeed won't work. The plugin does convert the element from <mermaid> to <div class='mermaid'> but the mermaid library imported from the script is probably not initialized at the right timing for those components. I tested a few variations but was not able to find one without any issues.

If the issue is on mermaid's side, then perhaps we can add a note to the user guide to mention this limitation.

Probably not exactly a mermaid library issue.

Do you think it's ok that I add the limitation and include a tracking issue first for the PR to be considered for merging?

@damithc
Copy link
Contributor

damithc commented Dec 23, 2022

Nice work @tlylt
It would be great to support mermaid diagrams, as I suspect they are lighter to use compared to PlantUML.

Probably a separate issue but it seems like mermaid does not work in a modal as well

A modal/panel can include content from any arbitrary file. If that file contains a mermaid diagram, it will not show up correctly in the modal? If this is the case, the problem is a serious one. For example, many of our MarkBind sites have panels/modal that include content from other places. In that case the diagrams will appear in some places and will not appear in other places of the site. Plus using a mermaid diagram will get in the way of the future reuse of that content elsewhere in the site.

@tlylt
Copy link
Contributor Author

tlylt commented Dec 23, 2022

A modal/panel can include content from any arbitrary file. If that file contains a mermaid diagram, it will not show up correctly in the modal? If this is the case, the problem is a serious one. For example, many of our MarkBind sites have panels/modal that include content from other places. In that case the diagrams will appear in some places and will not appear in other places of the site. Plus using a mermaid diagram will get in the way of the future reuse of that content elsewhere in the site.

Thanks @damithc for the input. Hmm in that case I think the current setup will indeed fail to work quite unexpectedly if panel/ modal + include is used. I will put this on hold for now until the problem is fixed then.

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Dec 30, 2022

Would this work? https://mermaid.js.org/config/usage.html#calling-mermaid-init

(for (re)-rendering as appropriate)

@tlylt
Copy link
Contributor Author

tlylt commented Dec 31, 2022

Would this work? mermaid.js.org/config/usage.html#calling-mermaid-init

(for (re)-rendering as appropriate)

Hi @ang-zeyu, thanks for pitching in. The init usage is marked as deprecated:

Warning This type of integration is deprecated. Instead the preferred way of handling more complex integration is to use the mermaidAPI instead.

I tried a few variations of calling initialize differently (calling it with startOnLoad = false, in an event listener to the page load event, after a 5-second timeout, calling it twice, etc). The result is that there are either no effects or the feature just stops working completely.

@ang-zeyu
Copy link
Contributor

I see, I missed that part... 😞

What about the API mentioned in the warning?

@tlylt
Copy link
Contributor Author

tlylt commented Dec 31, 2022

What about the API mentioned in the warning?

Do you mean the render method?

Currently, with the mermaid.initialize({'startOnLoad: true'}) the normal divs containing the mermaid class will render ok.
Then I tried experimenting with a settimeout to queryselect a div in panel and mermaid.render and setinnerhtml to replace the div with graph svg, and that seems to work...

I guess when the page is loaded, the content in vue components (panel, modal etc) is not yet ready. So when I do a queryselect
on load, it resulted in an error.

@ang-zeyu
Copy link
Contributor

Yes, I think startOnLoad: true seems more meant for simple use cases. We'll need to handle everything manually with startOnLoad: false and one of the more fine grained APIs.

Consider making this a vue component to handle the interactivity naturally. We don't have any options for adding Vue.plugins to our plugin system too yet unfortunately, adding infrastructure for that could be an optional prerequisite step.

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Dec 31, 2022

I will put this on hold for now until the problem is fixed then.

From Mermaid's perspective of startOnLoad: true, I don't think this is a bug too since it just scans the page on load once.

Tracking all mutations dynamically will likely be prohobitively expensive... 😞

https://stackoverflow.com/questions/31659567/performance-of-mutationobserver-to-detect-nodes-in-entire-dom

@tlylt
Copy link
Contributor Author

tlylt commented Jan 1, 2023

Consider making this a vue component to handle the interactivity naturally. We don't have any options for adding Vue.plugins to our plugin system too yet unfortunately, adding infrastructure for that could be an optional prerequisite step.

Alright, will experiment with that. One reason for choosing the plugin implementation is that it can be optional, hence users who are not interested can opt-out of that mermaid library import. Not too sure if doing it via component can also allow for that.

@tlylt tlylt marked this pull request as draft January 26, 2023 01:46
@jovyntls
Copy link
Contributor

A hacky fix could be to add the expanded attribute to all panels and then undo this at the final stage (postRender), but I'm not sure if this would cause hydration issues - it also won't fix potential problems any other components that may have this issue (popovers?).

Would it be worth expanding our plugin infrastructure? Specifically, adding APIs for preProcessNode and postProcessNode, which plugin developers can use to add more targeted node transformations. Though I'm not sure if this is necessary since most plugins might not need such a level of granularity 🤔

@damithc
Copy link
Contributor

damithc commented Feb 12, 2023

Mermaid's GitGraph feature https://mermaid.js.org/syntax/gitgraph.html is good enough justification to support it in MarkBind. So, looking forward to the day we can add this feature for good.

BTW, if the normal integration method doesn't work, does it make sense to somehow pre-generates the images during the build process and replace the mermaid code with the image instead?

@tlylt
Copy link
Contributor Author

tlylt commented Feb 13, 2023

@MarkBind/contributors hi guys as discussed, feel free to take a look at this draft PR and follow up if you are interested.

@tlylt tlylt mentioned this pull request Apr 2, 2023
4 tasks
@tlylt
Copy link
Contributor Author

tlylt commented Apr 2, 2023

I included the TODO in the chart support issue, for anyone interested in implementing this.

@tlylt tlylt closed this Apr 2, 2023
@Tim-Siu Tim-Siu mentioned this pull request Mar 14, 2024
14 tasks
@Tim-Siu Tim-Siu mentioned this pull request Mar 25, 2024
14 tasks
@Tim-Siu Tim-Siu mentioned this pull request Apr 24, 2024
14 tasks
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.

5 participants