-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
HTML escape included mermaid files to handle embedded tags #1580
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
Conversation
This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.
🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/BuaUAwE3i6YbCAWXNVXXgEERWosw
✅ Preview: https://docsify-preview-git-fork-mgranell-fix-embed-ac6ebc-docsify-core.vercel.app
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c539b25:
|
Test failed due to timeout, not related to change
Hi, @mgranell
Can you provide a screenshot of the before and after comparison?
I test here there is no difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should use a more robust solution like described in the following SO question so that it accounts for other characters, not just those specific ones?
https://stackoverflow.com/questions/18749591/encode-html-entities-in-javascript
Uh oh!
There was an error while loading. Please reload this page.
Summary
Fixes an issue where included mermaid files that had embedded tags failed to render, due to the file being included verbatim into the
<div>
tag, and embedded html within the mermaid file "breaking out" of the div.e.g. the following mermaid file will fail:
What kind of change does this PR introduce?
Bugfix
For any code change,
There are no tests that test in the include functionality
Does this PR introduce a breaking change? (check one)
Related issue, if any:
Tested in the following browsers: