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

Fix: Correctly render mermaid diagrams in markdown preview tool #4791

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

MaoShizhong
Copy link
Contributor

@MaoShizhong MaoShizhong commented Aug 28, 2024

Because

We initialise and run mermaid as soon as mermaid diagrams enter the DOM. If they have display: none, which they start off with as a child of a hidden parent element in the markdown preview page, they'll be rendered with incorrect SVG values - unable to rerender later as the mermaid text will have already been replaced.

This PR

  • Extract mermaid rendering to separate stimulus action
  • Runs diagramming#render when switching to rendered preview tab in the markdown preview tool
  • Moves the diagramming controller connect out of shared content component and into lesson show view
  • Replaces deprecated mermaid.init method with recommended new run method as per documentation for v10+

Issue

Closes #4783

Additional Information

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the keyword: brief description of change format, using one of the following keywords:
    • Feature - adds new or amends existing user-facing behavior
    • Chore - changes that have no user-facing value, refactors, dependency bumps, etc
    • Fix - bug fixes
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • I have verified all tests and linters pass after making these changes.
  • If this PR addresses an open issue, it is linked in the Issue section
  • If applicable, this PR includes new or updated automated tests

@MaoShizhong
Copy link
Contributor Author

MaoShizhong commented Aug 28, 2024

Edit: Fixed the below since I forgot I could just move the diagramming controller out of the content container component and into the lesson show view instead, since that connection no longer needs to be in a component shared between lesson and markdown preview.


Unable to remove diagramming controller from content_container_component.html.erb else diagrams in lesson pages get rendered incorrectly (funky colours).

Therefore, also unable to prevent diagramming#connect being called on the "Write" page of the Markdown Preview tool, hence the conditional query selector to prevent any mermaid diagrams getting rendered with incorrect SVG values before we actually want to see them.

This reverts commit 04aaf35.
For separate PR.
@MaoShizhong MaoShizhong changed the title Fix: Prevent rendering mermaid diagrams when display: none Fix: Correctly render mermaid diagrams in markdown preview tool Aug 31, 2024
…on show view

Was originally in the content container component to be shared between
the lesson view and markdown preview tool, but the markdown preview tool
needs a different connection/action structure to render mermaid diagrams
properly.

Moving it out of the content container means the diagramming#connect method
can be essentially returned to original using the default .mermaid selector.
@KevinMulhern KevinMulhern self-requested a review September 3, 2024 17:11
Copy link
Member

@KevinMulhern KevinMulhern left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @MaoShizhong 💪

It's a shame we can't keep the diagramming controller with the content container, but the trade off is worth it. It's a good solution! just one nit from me.

app/views/lessons/previews/show.html.erb Outdated Show resolved Hide resolved
Copy link
Member

@KevinMulhern KevinMulhern left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 nice work @MaoShizhong!

@MaoShizhong MaoShizhong merged commit 2110ab1 into TheOdinProject:main Sep 4, 2024
5 checks passed
@MaoShizhong MaoShizhong deleted the fix-mermaid-preview branch September 4, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Bug: Mermaid diagrams not working on lesson previews
2 participants