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.

Sign up for GitHub

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

Jump to bottom

Update diagrams on terms.md #595

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update diagrams on terms.md #595

wants to merge 1 commit into from

Conversation

samerfarida
Copy link

Updated the simple and advanced diagrams as a code using mermaid

@samerfarida
Update terms.md
5f2f03d 
Updated the simple and advanced diagrams  as a code using mermaid
samerfarida
samerfarida commented Aug 25, 2024
View reviewed changes
Copy link
Author

@samerfarida samerfarida left a comment

Choose a reason for hiding this comment

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

I have reviewed this code. Is there anything else you need from me?

@samerfarida samerfarida changed the title Update terms.md Update diagrams on terms.md Aug 25, 2024
@danny6167 danny6167 self-assigned this Sep 11, 2024
@rowansc1
Copy link
Contributor

Sorry for the wait @samerfarida! This will get looked into.

@rowansc1 rowansc1 added the enhancement New feature or request label Sep 29, 2024
@danny6167
Copy link
Member

Is there something missing from your PR?
Seems this mermaid stuff isn't something it knows how to render.
https://pr-595.ptero-doc-preview.pages.dev/project/terms.html

@samerfarida
Copy link
Author

Is there something missing from your PR? Seems this mermaid stuff isn't something it knows how to render. https://pr-595.ptero-doc-preview.pages.dev/project/terms.html

There is no issue with the current PR itself. However, I wasn't aware that the .md file would be converted to .html, which introduces the rendering problem. After reviewing the Mermaid.js documentation at https://mermaid.js.org/config/usage.html#simple-full-example, it seems that in order to properly render Mermaid diagrams in HTML, the code needs to be wrapped as follows:

<!-- Other HTML code -->

<pre class="mermaid">
    <!-- Insert Mermaid code here -->
</pre>
<script type="module">
    import mermaid from 'https://cdn.jsdelivr.net/npm/mermaid@11/dist/mermaid.esm.min.mjs';
</script>

<!-- More HTML code -->

For example, I have attached an .html file that demonstrates this. Please download it (rename it by removing the .txt extension) and open it locally in a browser to see a Mermaid diagram rendered within HTML. Below is a sample of the Mermaid code inside the HTML structure:

mermaidTest.html.txt

<!doctype html>
<html lang="en">
  <body>
    <pre class="mermaid">
      graph LR
      A --- B
      B-->C[fa:fa-ban forbidden]
      B-->D(fa:fa-spinner);
    </pre>
    <script type="module">
      import mermaid from 'https://cdn.jsdelivr.net/npm/mermaid@11/dist/mermaid.esm.min.mjs';
    </script>
  </body>
</html>

At this point, we have three possible options moving forward:

  1. Save the Mermaid diagrams as .png files and replace the current diagrams. This way, they will render correctly on both GitHub and the website.
  2. Modify the build process to wrap the Mermaid code in the required HTML structure during the conversion from .md to .html, as recommended by Mermaid.js.
  3. I can update the current PR to include the necessary HTML structure. However, this will prevent the Mermaid code from rendering correctly in the .md file on GitHub, though it will work on the website.

Please let me know which option you and the Pterodactyl team would prefer.

Thanks,
Sammy

@rowansc1
Copy link
Contributor

rowansc1 commented Oct 3, 2024

Hi Sammy,

Thank you very much for that detailed analysis.

Personally, I think option 2 is the most preferable. However, option 3 would also still be fine as the docs are meant to be viewed on the website anyway.

What do you think @danny6167 ?

@danny6167
Copy link
Member

I really don't want to see us adding little hacks to the build process for things like this.
In-lining HTML, especially with external dependencies (something we don't do) really isn't a great option either.

pre-rendered images is really the only valid option of the options that's been listed. But the ideal solution would be if there was a native solution for vuepress to be able to render the mermaid syntax.

It really doesn't matter how it looks in github, what matters is how it looks when it's tested after the vuepress build.

@danny6167 danny6167 added the Please Correct Waiting for user to fix the requested issues label Oct 3, 2024
@samerfarida
Copy link
Author

I really don't want to see us adding little hacks to the build process for things like this. In-lining HTML, especially with external dependencies (something we don't do) really isn't a great option either.

pre-rendered images is really the only valid option of the options that's been listed. But the ideal solution would be if there was a native solution for vuepress to be able to render the mermaid syntax.

It really doesn't matter how it looks in github, what matters is how it looks when it's tested after the vuepress build.

Hey @danny6167, I have no problems with pre-rendered images. I did a quick search on https://vuepress.vuejs.org/ and it looks like they have Markdown plugins that can be enabled.

Apparently, they do have a Mermaid plugin for your VuePress site. Please review. It looks like it’s an easy install and enablement. This way, you guys are set for future usages of diagrams as code!

Please let me know if you still want me to correct this PR to include a pre-rendered images?

Thank you,

Sammy

@danny6167
Copy link
Member

That's a good find @samerfarida - I'll take a look at that and update soon if it works out.

@danny6167
Copy link
Member

Hi!

Just letting you know I haven't forgotten about this.

We can't merge this right now. Looks like we are going to have to update some dependencies, possibly even move to the next version of vuepress to be able to support this properly. That's going to take some time.

Leave the PR open, and we will merge it when the rest of the code is ready for it.

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@danny6167 danny6167

Labels
enhancement New feature or request Please Correct Waiting for user to fix the requested issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@samerfarida @rowansc1 @danny6167