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 Plugin #2475

Merged
merged 37 commits into from Apr 24, 2024
Merged

Add Mermaid Plugin #2475

merged 37 commits into from Apr 24, 2024

Conversation

Tim-Siu
Copy link
Contributor

@Tim-Siu Tim-Siu commented Mar 25, 2024

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:
Related to #2052
Related to #1663 for pie-chart
Related to #984 for diagramming tool

This pull request is a continuation of the work done in the previously closed pull request #2052. It builds upon the changes and addresses the diagrams not rendered inside Vue components issue.

I would like to acknowledge and give credit to the original author, @tlylt , for his initial contribution and efforts in the closed pull request. Their work laid the foundation for the changes proposed in this new pull request.

Some changes include:

  1. Upgrade the usage of Mermaid to v10.
  2. Migrate JavaScript to TypeScript.
  3. Drop the configuration customization as it seems no longer relevant (given loading issues are solved)

Please review the changes and provide any further feedback or suggestions for improvement. Thank you!

Anything you'd like to highlight/discuss:

function genScript(address: string) {
  return `<script type="module">
    import mermaid from '${address || DEFAULT_CDN_ADDRESS}';
    document.addEventListener('DOMContentLoaded', () => {
      Vue.directive('mermaid', {
        inserted: function(el) {
          mermaid.run({
            nodes: [el]
          });
        }
      });
    });
  </script>`;
}

Based on my ''ablation studies'', it seems like that importing the script will results in automatic initialization (for diagrams outside of Vue components) and the Vue directive will initialize the diagrams inside Vue components.

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)

Support Mermaid


Checklist: ☑️

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

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.91%. Comparing base (c9f72c4) to head (3186303).

❗ Current head 3186303 differs from pull request most recent head c6c869b. Consider uploading reports for the commit c6c869b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2475      +/-   ##
==========================================
- Coverage   51.05%   50.91%   -0.14%     
==========================================
  Files         126      126              
  Lines        5467     5450      -17     
  Branches     1174     1171       -3     
==========================================
- Hits         2791     2775      -16     
+ Misses       2381     2380       -1     
  Partials      295      295              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Tim-Siu Tim-Siu marked this pull request as ready for review April 1, 2024 04:46
@kaixin-hc
Copy link
Contributor

Nice work! It looks good and the documentation is clear - though I'd like to play around with it a little more before approving

I am wondering if we should have it be a separate test site, as this is also a plugin similar to algolia and the like. We should probably come to a decision and then include this in the developer guide for guidance

@EltonGohJH
Copy link
Contributor

I think implementation look good to me. Just like Hannah, I need more time to play around and test it!

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Ok the mermaid website is broken so I can't test it but some nits about documentation first!

docs/userGuide/plugins/mermaid.md Outdated Show resolved Hide resolved
docs/userGuide/plugins/mermaid.md Outdated Show resolved Hide resolved
@@ -0,0 +1,96 @@
### Plugin: Mermaid

This plugin allows you to utilize <tooltip content="Mermaid lets you create diagrams and visualizations using text and code">Mermaid</tooltip> by automatically importing the library and initializing the rendering of the diagrams.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think linking to the Mermaid documentation here is better than using a tool tip

Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhat agree but I really liked the effect of the tooltip as well. Maybe at the bit where we talk about external plugins, we can add tooltips to the documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tooltip has been removed. Looking for more advice.

@yucheng11122017 yucheng11122017 mentioned this pull request Apr 4, 2024
14 tasks
Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Hi @Tim-Siu thanks for the PR! The test site doesn't seem to be working... could you check?

docs/userGuide/syntax/diagrams.md Outdated Show resolved Hide resolved
Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Some nits and improvements on test case! This looks great the mermaid diagrams look really awesome

@@ -0,0 +1,71 @@
**Mermaid Test**
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a functional test to make sure that mermaid works when using include?

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the mermaid plugin loads after the two internal and external plugin we have on the test site, so it will like this at first..
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the mermaid plugin loads after the two internal and external plugin we have on the test site, so it will like this at first.. image

Thanks for the investigation!

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I mistook it for a unit test and was thinking about how to achieve this. Thank you for your reminder!

docs/userGuide/syntax/diagrams.md Outdated Show resolved Hide resolved
docs/userGuide/plugins/mermaid.md Show resolved Hide resolved
@EltonGohJH
Copy link
Contributor

LGTM. I think the implementation is good.
I think should be good to merge after you resolve all the comments pointed out by @yucheng11122017 .

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Some nits! Also @Tim-Siu when modifying all your PRs could you please make sure that you address all the comments on the review? If you disagree with it, you can reply on why you disagree rather than leaving it.

docs/userGuide/plugins/mermaid.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/diagrams.md Outdated Show resolved Hide resolved
@@ -0,0 +1,71 @@
**Mermaid Test**

This comment was marked as resolved.

@Tim-Siu
Copy link
Contributor Author

Tim-Siu commented Apr 21, 2024

Some nits! Also @Tim-Siu when modifying all your PRs could you please make sure that you address all the comments on the review? If you disagree with it, you can reply on why you disagree rather than leaving it.

Sorry for the confusion, I will keep clearer communications next time.

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

One comment and i think should be good!

packages/cli/test/functional/test_site/testMermaid.md Outdated Show resolved Hide resolved
Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Some comments

@@ -65,6 +65,8 @@ MarkBind has a set of built-in plugins that can be used immediately without inst
<include src="plugins/mathDelimiters.md" />
<include src="plugins/web3Form.md" />
<include src="plugins/dataTable.md" />
<include src="plugins/mermaid.md" />

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary new line


<include src="testMermaidFragment.md" />

<panel type="minimal" header="This is to test Mermaid diagrams works when included inside a panel." src="testMermaidFragment.md">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<panel type="minimal" header="This is to test Mermaid diagrams works when included inside a panel." src="testMermaidFragment.md">
<panel type="minimal" header="This is to test Mermaid diagrams works when included inside a panel." src="testMermaidFragment.md"/>

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use the word fragment here because include fragment means like a partial include

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

LGTM!

@yucheng11122017 yucheng11122017 merged commit 004e7d2 into MarkBind:master Apr 24, 2024
8 checks passed
@github-actions github-actions bot added the r.Minor Version resolver: increment by 0.1.0 label Apr 24, 2024
@damithc
Copy link
Contributor

damithc commented Apr 24, 2024

@Tim-Siu Good work on adding this. While we are on this topic, is there a reason (e.g., performance cost) for not enabling this plugin (and the data table plugin) by default? Normally, we want to make 'everything works out of the box' (i.e., requires minimal configuration by the user.

@Tim-Siu
Copy link
Contributor Author

Tim-Siu commented Apr 24, 2024

@Tim-Siu Good work on adding this. While we are on this topic, is there a reason (e.g., performance cost) for not enabling this plugin (and the data table plugin) by default? Normally, we want to make 'everything works out of the box' (i.e., requires minimal configuration by the user.

Hi @damithc ,

For the mermaid plugin, the external javascript loaded from CDN comes in at 3.3MB and is kind of heavy. It may results in some network costs.

For the dataTable plugin, the javascript involved is much lighter. I have not systematically determined the performance overhead though.

@damithc
Copy link
Contributor

damithc commented Apr 24, 2024

For the mermaid plugin, the external javascript loaded from CDN comes in at 3.3MB and is kind of heavy. It may results in some network costs.

For the dataTable plugin, the javascript involved is much lighter. I have not systematically determined the performance overhead though.

@Tim-Siu I see. Is an 'enable by default, but don't load if not used' approach feasible?

@Tim-Siu
Copy link
Contributor Author

Tim-Siu commented Apr 24, 2024

For the mermaid plugin, the external javascript loaded from CDN comes in at 3.3MB and is kind of heavy. It may results in some network costs.
For the dataTable plugin, the javascript involved is much lighter. I have not systematically determined the performance overhead though.

@Tim-Siu I see. Is an 'enable by default, but don't load if not used' approach feasible?

I think it is possible to achieve this in the page build stage. I can do some research and work on it after the exams.

@damithc
Copy link
Contributor

damithc commented Apr 24, 2024

I think it is possible to achieve this in the page build stage. I can do some research and work on it after the exams.

@Tim-Siu I see. Perhaps, something to try in future. Perhaps create an issue for it now so that we keep it in view?

@Tim-Siu
Copy link
Contributor Author

Tim-Siu commented Apr 24, 2024

I think it is possible to achieve this in the page build stage. I can do some research and work on it after the exams.

@Tim-Siu I see. Perhaps, something to try in future. Perhaps create an issue for it now so that we keep it in view?

Sure!

itsyme added a commit that referenced this pull request Apr 24, 2024
Co-authored-by: Tim-Siu <shuyao@u.nus.edu>
Co-authored-by: yucheng2207 <e0560244@u.nus.edu>
Co-authored-by: david <71922282+itsyme@users.noreply.github.com>
@damithc
Copy link
Contributor

damithc commented May 3, 2024

Feature in action: https://nus-cs2103-ay2324s2.github.io/website/schedule/week3/project.html

image

https://nus-cs2103-ay2324s2.github.io/website/admin/gradeBreakdown.html

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r.Minor Version resolver: increment by 0.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants