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

Mermaid Master Ticket (Improvements and Issues) #7629

Open
6 of 8 tasks
jasonwilliams opened this issue Jan 16, 2023 · 26 comments
Open
6 of 8 tasks

Mermaid Master Ticket (Improvements and Issues) #7629

jasonwilliams opened this issue Jan 16, 2023 · 26 comments
Labels
bug It's a bug enhancement Feature requests and code enhancements medium Medium priority issues renderer About the note renderer

Comments

@jasonwilliams
Copy link
Contributor

jasonwilliams commented Jan 16, 2023

This ticket is a master ticket to keep track of the most important Mermaid issues.

It also continues the discussion made in #7330 which goes through outstanding issues with Mermaid integration.

CC @tessus @AelithBlanchett

@jasonwilliams jasonwilliams added the bug It's a bug label Jan 16, 2023
@tessus tessus added enhancement Feature requests and code enhancements renderer About the note renderer medium Medium priority issues labels Jan 16, 2023
@tessus
Copy link
Collaborator

tessus commented Jan 17, 2023

After looking into theming something came up that could make the dark theme support a bit more complicated:

When you look at Mermaid's dark theme, it turns out that the background is still white. Here is an example in the Live Editor. On the other side I haven't changed the Joplin code yet to test it within Joplin. Maybe the background will adjust. The background attribute in Mermaid states it is used to calculate color for items that should either be background colored or contrasting to the background.

IMO someone would have to create a palette themselves that we can then use for dark theme.

Wrt images in flowcharts: I was also looking into this and I can't even make it work in the live editor. So I am not sure how I came up with that problem in the first place. It might have worked with earlier versions of Mermaid, but either way, I haven taken this item off the list.

@tessus tessus changed the title Mermaid Improvements Mermaid Master Ticket (Improvements and Issues) Jan 17, 2023
@jasonwilliams
Copy link
Contributor Author

Thanks for editing this issue @tessus

When you look at Mermaid's dark theme, it turns out that the background is still white.

It doesn't look white to me from that link:
mermaid

The window on the left is Joplin. I'm sure its possible for us to create a custom theme anyway right? It may take some time to put together the right colours but I don't think this should be an issue.

If you can locate me where in this codebase this is set I'm happy to look into creating a custom palette.

@tessus
Copy link
Collaborator

tessus commented Jan 17, 2023

Hmm, in my browser it looks like this:

image

Anyway, I'll run a few tests during the week.

@sidharthv96
Copy link

@KrullTheWarriorKing
Copy link

You can refer sample mindmap integration https://github.com/mermaid-js/mermaid-live-editor/blob/fcf53c98c25604c90a218104268c339be53035a6/src/lib/util/mermaid.ts

Also, from our exchange, @sidharthv96 added : "mindmap is async, and requires a separate package (for now). Would that be the issue?"

@KrullTheWarriorKing
Copy link

And from Alios from Mermaid :

It was a bit of a pain/hack to get mindmaps working in mermaid-cli project too.
What we ended up doing was to first create an index.html file that had an import mermaidMindmap from "@mermaid-js/mermaid-mindmap". This is so that our bundler could find the mindmap plugin.
Then, before running Mermaid, we did an await mermaid.registerExternalDiagrams([mermaidMindmap]) to register the mindmap plugin with mermaid.
https://github.com/mermaid-js/mermaid-cli/blob/67ed625e900e84249dc4afe08b2ecca5392874a0/src/index.js#L218-L223

@jasonwilliams
Copy link
Contributor Author

jasonwilliams commented Feb 23, 2023

Mermaid 10 has just come out https://github.com/mermaid-js/mermaid/blob/develop/CHANGELOG.md#1000
It is esm only, does anyone know if that would be an issue to use here? @tessus I think you handled the last update.
It is also async, so may need to use top-level await in some places but Im sure we're using an up to date version of electron so this shouldn't be an issue

@tessus
Copy link
Collaborator

tessus commented Feb 24, 2023

This is a good question. Unfortunately I am currently a bit busy with other things.

@sidharthv96
Copy link

Looking at mermaid_render.js, the new update shouldn't be a major issue. It'll even work as-is if we inject mermaid properly as the init method will still work.

The only issue would be how the assets are injected.

@tessus
Copy link
Collaborator

tessus commented Feb 25, 2023

@sidharthv96 Electron does not support ESM.

@jasonwilliams
Copy link
Contributor Author

jasonwilliams commented Feb 26, 2023

Yep electron/electron#21457

Taking a quick look at the mermaid exported code I don't see it working at all in Electron. Whilst I do support the migration to ESM, being "esm only" may be a bit too soon when environments like Electron don't support it yet. Id still personally be rolling out a hybrid approach. But that's just me.

Also, @sidharthv96, there seem to be a lot of files in the mermaid package. Are you outputting a single minified version of the mermaid package? Or am i just seeing hashes of previous versions in there? There's a runtime performance penalty of Node needing to load many files (I briefly talk about it here: https://jason-williams.co.uk/posts/speeding-up-vscode-extensions-in-2022/)

So it looks like the move to 10.0.0 will be blocked for quite a while

@tessus
Copy link
Collaborator

tessus commented Feb 26, 2023

see also https://github.com/orgs/mermaid-js/discussions/4148

Unfortunately I haven't received an answer from the devs yet.

@wh201906
Copy link
Contributor

wh201906 commented Apr 15, 2023

@tessus Hi. Is this helpful for Joplin?
mermaid-js/mermaid#4281
I don't know if the CJS-style require("") is necessary for Joplin. The developer said he won't support it unless there's a good reason.
mermaid-js/mermaid#4281 (comment)

I haven't yet seen people asking for the CJS-style require("") back, and so I'm reluctant to add back support for it unless there's a good reason, as it will probably causes CJS/ESM typescript bugs like #3799 to be re-opened.

@tessus
Copy link
Collaborator

tessus commented Apr 15, 2023

I am sorry, but I'm currently unable to look into any of this. Maybe Laurent or somebody else can find some time.

@laurent22
Copy link
Owner

  • Mindmaps don't work
  • tooltips don't work
  • fa icons
  • Support dark mode theme

If there's no issue for these please add one, so that we can keep track of it (and have more details on how to replicate it)

@tessus
Copy link
Collaborator

tessus commented Aug 25, 2023

Yes, this was my plan, but then got side-tracked with other stuff. I will open them on the weekend.

@wh201906
Copy link
Contributor

Hey I guess we can keep using the mermaid.min.js now. The CJS support is added since v10.2.0
mermaid-js/mermaid@04305bd
And @oj-lappi has made a PR for it
#8890
@tessus Could you please take a look at it?

@tessus
Copy link
Collaborator

tessus commented Sep 16, 2023

A few assets are missing. The usual process for updating Mermaid is as follows:

  • Update the version in packages/renderer/package.json
  • run yarn install in the root dir (to install the new Mermaid package)
  • go to packages/renderer/ and run npm run buildAssets (creates the new asset)
  • yarn postinstall in the root dir (this creates the mobile files)

I haven't had a chance to test the PR yet, because I am on the road until Tuesday night and my mind is all over the place. I will have a look at it Wednesday afternoon unless somwbody else beats me to it.

oj-lappi added a commit to oj-lappi/joplin that referenced this issue Sep 17, 2023
 - timeline chart
 - quadrant charts
 - Sankey diagram

Note: completely new commit on most recent upstream dev, just reran all steps outline by tessus's in laurent22#7629
@jasonwilliams
Copy link
Contributor Author

  • Mindmaps don't work
  • tooltips don't work
  • fa icons
  • Support dark mode theme

If there's no issue for these please add one, so that we can keep track of it (and have more details on how to replicate it)

@laurent22 rather than me create a new issue for dark mode, could you re-open #3201 and I can link it from here? Thanks

@jasonwilliams
Copy link
Contributor Author

Ok status update

  1. All: Resolves #8728: Bump mermaid version to 10.4.0 to support new chart types #8890 Has had reviews from myself and @tessus, I guess it can be merged or @laurent22 may also want to review. But it seems ready to go
  2. Tooltips and fa icons have no issue (and I don’t know the context of them). So we either remove them from the top list or make issues for them (I believe @tessus said he was going to do this)
  3. Dark Mode issue has been re-opened. Anyone should be able to pick that up, there’s info in this issue on how it could be done, it should be straight forward but a new palette would need to be created

@AjayDextrous

This comment was marked as off-topic.

@laurent22

This comment was marked as off-topic.

@AjayDextrous

This comment was marked as off-topic.

@laurent22

This comment was marked as off-topic.

@fanecovi
Copy link

fanecovi commented Jan 5, 2024

Issue #9588 should be added into this master ticket as well

@bertini97
Copy link

Just a heads up that Mermaid 10.9.0 has been out. Joplin is on 10.6.1 and since this a lot of stuff has been added, including more graph types that would be nice to have. Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug enhancement Feature requests and code enhancements medium Medium priority issues renderer About the note renderer
Projects
None yet
Development

No branches or pull requests

9 participants