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

Add mermaid support #5383

Closed
wants to merge 4 commits into from
Closed

Add mermaid support #5383

wants to merge 4 commits into from

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Feb 22, 2024

This pull request adds support for mermaid in dodona.
It adds mermaid as a js dependency. In the backend I added a custom kramdown modification based on this gitlad_kramdown commit.
I prefer this custom solution over using gitlab_kramdown because I was not able to disable the gitlab specific references.

Mermaid js is a relevant increase in our js package size
image

But this impact is limited as everything is dynamically loaded. Nothing is loaded when no mermaid graphs are on the page.

In most examples I tried, only the package 9465 (674kb unminimized and 295kb minimized) was ever loaded. This package contains the main mermaid and some d3 code.
The packages containing cytoscape and elkjs are probably only loaded for specific graphs when needed.

  • check package size increase
  • only load mermaid on relevant pages
  • avoid gitlab links being inserted

image
image

Closes #4168

@jorg-vr jorg-vr added the feature New feature or request label Feb 22, 2024
@jorg-vr jorg-vr self-assigned this Feb 22, 2024
@jorg-vr jorg-vr marked this pull request as ready for review February 22, 2024 16:30
@jorg-vr jorg-vr requested a review from a team as a code owner February 22, 2024 16:30
@jorg-vr jorg-vr requested review from bmesuere and niknetniko and removed request for a team February 22, 2024 16:30
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

Seems to work. One niche issue:

  • When changing dark/light mode, the mermaid.js stuff is not update to dark/light mode: it needs a hard refresh.

Would it be an idea to include a basic test for the markdown stuff so it doesn't break unnoticed? Something like:

kramdown_test.rb
require 'test_helper'

class KramdownTest < Minitest::Test
  include ApplicationHelper

  def test_mermaid_works
    markdown = <<~EOS
      This is an example of codeblocks using mermaid:

      ```mermaid
      graph TD;
        MoreUsers-->MorePipeline;
        MorePipeline-->MoreRevenue;
        MoreRevenue-->MoreFeatures;
        MoreFeatures-->MoreUsers;
      ```
    EOS

    expected_html = <<~EOS
      <p>This is an example of codeblocks using mermaid:</p>

      <div class="mermaid">graph TD;
        MoreUsers-->MorePipeline;
        MorePipeline-->MoreRevenue;
        MoreRevenue-->MoreFeatures;
        MoreFeatures-->MoreUsers;
      </div>
    EOS

    actual_html = markdown_unsafe(markdown)

    assert_equal expected_html, actual_html
  end
end

@bmesuere
Copy link
Member

This has a way bigger impact than I expected. We should discuss if we really want to add this. Unfortunately, there doesn't seem to be a server-side option.

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Feb 23, 2024

@niknetniko
I noticed this as well, but there is no mermaid support to change the active theme. mermaid-js/mermaid#1945
One of the comments does describe a workaround, but I do not think it relevant for such a niche issue. We are probably the only ones who regularly switch themes, these graphs will only be present on a small subset of pages, and it is just a minor visual issue.

I will add the test, thanks :)

@bmesuere I'll drop by to discuss

@jorg-vr jorg-vr closed this Mar 4, 2024
@jorg-vr jorg-vr deleted the feat/add-mermaid branch April 22, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Mermaid 🧜‍♀️ to Markdown parser
3 participants