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

ERD diagram of mermaid is not rendered #3108

Closed
tim-hub opened this issue Apr 21, 2020 · 25 comments · Fixed by #3745
Closed

ERD diagram of mermaid is not rendered #3108

tim-hub opened this issue Apr 21, 2020 · 25 comments · Fixed by #3745
Labels
enhancement Feature requests and code enhancements

Comments

@tim-hub
Copy link

tim-hub commented Apr 21, 2020

Play round the mermaid diagram, found the ERD is not rendered.

image

Environment

Joplin version: 1.0.201
Platform: Mac OS
OS specifics: (prod, darwin) Mac OS Catalina

Steps to reproduce

  1. follow the screen shot, create a ERD diagram

Describe what you expected to happen

the ERD should be rendered in preview windows.

Logfile

please see the screenshot above.

@tim-hub tim-hub added the bug It's a bug label Apr 21, 2020
@laurent22
Copy link
Owner

Why not provide the Markdown you used so that we can replicate the issue?

@coderrsid
Copy link
Contributor

Markdown to recreate the issue

# erd
```mermaid
erDiagram
	CUSTOMER ||--o{ ORDER : places
	ORDER ||--|{  LINE-ITEM : contains
	CUSTOMER }|..|{ DELIVERY-ADDRESS : uses

@coderrsid
Copy link
Contributor

Although @tim-hub , these are experimental features of Mermaid-js-erd

image

@tessus
Copy link
Collaborator

tessus commented Apr 21, 2020

It's possible that our Mermaid version does not even have that feature yet.

@tessus
Copy link
Collaborator

tessus commented Apr 21, 2020

Yep, this was added in 8.5.0. We use 8.4.6.

@coderrsid
Copy link
Contributor

It's possible that our Mermaid version does not even have that feature yet.

You're perfectly right, tessus. This feature is added to v8.5.0 and we're using v8.4.6 . Everyone can see below in the changelog of mermaid-js Features section.

image

@tim-hub
Copy link
Author

tim-hub commented Apr 22, 2020

Why not provide the Markdown you used so that we can replicate the issue?

just forgot

@tim-hub
Copy link
Author

tim-hub commented Apr 22, 2020

Markdown to recreate the issue

# erd
```mermaid
erDiagram
	CUSTOMER ||--o{ ORDER : places
	ORDER ||--|{  LINE-ITEM : contains
	CUSTOMER }|..|{ DELIVERY-ADDRESS : uses

thanks

@tim-hub
Copy link
Author

tim-hub commented Apr 22, 2020

@laurent22 @coderrsid @tessus

thanks for the finding out

@tessus
Copy link
Collaborator

tessus commented Apr 22, 2020

No worries, at one point we'll update Mermaid. No ETA though.

@tessus
Copy link
Collaborator

tessus commented Apr 22, 2020

@laurent22 I actually tried to update mermaid, but none of the scripts updates the mermaid.js in rnInjectedJs. What am I missing?

I tried npm i in the root folder, npm run build in ElectronClient and ReactNativeClient.

Update: Never mind, I figured it out, but it actually resulted in more questions:
Why is the mermaid package in the package.json of ElectronClient and ReactNativeClient, while all where it is needed is in ReactNativeClient/lib/joplin-renderer/package.json.

@stale
Copy link

stale bot commented Jun 6, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

@stale stale bot added the stale An issue that hasn't been active for a while... label Jun 6, 2020
@tessus
Copy link
Collaborator

tessus commented Jun 6, 2020

Mermaid has to be updated. Upstream already fixed the issue I encountered after upgrading to 8.5.0.
So we should be good to go.

I'll submit a PR tomorrow. I'm currently not home.

@stale stale bot removed the stale An issue that hasn't been active for a while... label Jun 6, 2020
@tessus
Copy link
Collaborator

tessus commented Jun 7, 2020

I've updated Mermaid to 8.5.2, but the squence diagram and the gantt chart are too small. I suspect that we have previously added additional css to Joplin to fix upstream issues, but I don't know.

@laurent22 do you remember adding fixes for Mermaid upstream bugs that had something to do with the size of the mermaid images?

image

@stale
Copy link

stale bot commented Jul 10, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

@stale stale bot added the stale An issue that hasn't been active for a while... label Jul 10, 2020
@tessus
Copy link
Collaborator

tessus commented Jul 10, 2020

@laurent22 do you remember adding fixes for Mermaid upstream bugs that had something to do with the size of the mermaid images?

@stale stale bot removed the stale An issue that hasn't been active for a while... label Jul 10, 2020
@stale
Copy link

stale bot commented Aug 9, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

@stale stale bot added the stale An issue that hasn't been active for a while... label Aug 9, 2020
@tim-hub
Copy link
Author

tim-hub commented Aug 9, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

i think it still not fixed

@stale stale bot removed the stale An issue that hasn't been active for a while... label Aug 9, 2020
@stale
Copy link

stale bot commented Sep 9, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

@stale stale bot added the stale An issue that hasn't been active for a while... label Sep 9, 2020
@tessus
Copy link
Collaborator

tessus commented Sep 9, 2020

The problem is that new versions of Mermaid require a change from fit-content to 100% for the width. But this also results in another issue described here: https://discourse.joplinapp.org/t/upgrading-mermaid/10140/4?u=tessus
However, since Laurent changed it to 100%, we can also upgrade Mermaid....

@stale stale bot removed the stale An issue that hasn't been active for a while... label Sep 9, 2020
@laurent22
Copy link
Owner

Isn't it the same issue as this one? #3097 I think the fix would be to make the graph scrollable rather than 100% width, so that we can handle graphs of any size.

@tessus
Copy link
Collaborator

tessus commented Sep 14, 2020

Well, the fit-content did that, but then some of the graphs are too small. I explained this in the topic referenced.
So I think at one point you added the fit-content but now after mermaid 8.5.0 some images are too small. Going back to 100% fixes the size problem, but then the scroll issue resurfaces....

@tessus tessus linked a pull request Sep 16, 2020 that will close this issue
@tessus tessus added enhancement Feature requests and code enhancements and removed bug It's a bug labels Sep 16, 2020
@stale
Copy link

stale bot commented Oct 17, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

@stale stale bot added the stale An issue that hasn't been active for a while... label Oct 17, 2020
@MHoeller
Copy link

The issue is fixed, with version 1.2.x since this was a release I considered this as tested.
In case it is needed: from my perspective this issue can be closed

@stale stale bot removed the stale An issue that hasn't been active for a while... label Oct 17, 2020
@tessus
Copy link
Collaborator

tessus commented Oct 17, 2020

Yep, Caleb added this with the bump to Mermaid 8.x.y

@tessus tessus closed this as completed Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and code enhancements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants