Add Mermaid Plugin - Githubissues

Tim-Siu commented 3 months ago

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:

Upgrade the usage of Mermaid to v10.
Migrate JavaScript to TypeScript.
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: :ballot_box_with_check:

[ ] 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)
[x] 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).

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kaixin-hc commented 3 months ago

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 commented 3 months ago

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

EltonGohJH commented 2 months ago

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

Tim-Siu commented 2 months ago

Hi @yucheng11122017 ,

I am not sure why but it seems like async loading is not compatible with the latest changes to hydration...

Tim-Siu commented 2 months ago

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.

damithc commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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!

damithc commented 2 months ago

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

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

MarkBind / markbind

Add Mermaid Plugin #2475

Codecov Report