-
-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
wip: Add optInMarkdownLabels config setting #6277
wip: Add optInMarkdownLabels config setting #6277
Conversation
|
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6277 +/- ##
==========================================
- Coverage 3.88% 3.88% -0.01%
==========================================
Files 398 399 +1
Lines 42044 42081 +37
Branches 638 638
==========================================
Hits 1635 1635
- Misses 40409 40446 +37
Flags with carried forward coverage won't be shown. Click here to find out more.
|
6bd09a9
to
2025302
Compare
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
I spent some more time looking through the project today. Looking through the e2e tests, unless I missed something, it seems the only time markdown rendering is tested is when it is within the double-quote-backtick delimiters. Also, I see other issues/PRs addressing the same, or at least highly similar, issue. For example: #5824 and associated PR #6087. Both this PR and that apply the change to pull the vertex labelType into the node (though they also do the same for the edges, which I had not yet done). Looking at the comments there, that PR seems idle for 3 months. My attempt here has had no maintainer feedback for 2 weeks. I'd be willing to either apply my config setting approach in this PR to theirs, or adopt their re-use of |
I'm inclined to withdraw this PR in favor of #6345. It seems a change without additional configuration, simply restoring the old behavior, was very close to merging recently. That PR wasn't quite sufficient for my needs, so I added some small changes in #6345. If the maintainers would prefer a configuration option, I can add that in #6345. |
📑 Summary
This is a quick strawman implementation of the direction I'm proposing to resolve #6275.
Resolves #6275
Examples of types of labels causing issues in an external project:
After applying new config setting with these changes:

📏 Design Decisions
I've added an
optInMarkdownLabels
config setting. It defaults to false, but when enabled (either in frontmatter, or in a call to initialize), it turns off the auto-markdown processing for node labels.The primary goal is to provide a less intrusive upgrade path from Mermaid 10 to 11. Users who encounter markdown issues in their node rendering when upgrading to Mermaid 11 could set
optInMarkdownLabels
to false in order restore a closer approximation of the 10.x behavior. I'm not claiming this PR will ever guarantee no rendering changes in the upgrade from 10 to 11, but it will certainly provide a fix for theUnsupported markdown
issues that can be accidentally encountered during the upgrade.There are many issues to solve before the implementation could be considered for merge. I'm sharing the early draft just as a conversation point, to see if this approach would be acceptable if the following issues were resolved.
useHtmlLabels
path, which callsmarkdownToHTML
. I should probably update themarkdownToLines
path foruseHtmlLabels=false
as well.📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.