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

All: Resolves #8728: Bump mermaid version to 10.4.0 to support new chart types #8890

Merged
merged 1 commit into from Sep 19, 2023

Conversation

oj-lappi
Copy link
Contributor

@oj-lappi oj-lappi commented Sep 16, 2023

A bump from 9.2.2 to 10.4.0 adds at least the following chart types:

  • timeline chart
  • quadrant charts
  • Sankey diagrams (flow from one set of elements to another set, e.g. used in budget analysis)

Browsing through the forums I noticed some old topics on previous updates to the mermaid dependency.

https://discourse.joplinapp.org/t/upgrading-mermaid/10140/8

laurent, Jun '21:
Normally it's easy and even documented 30 but I guess no-one took the time to do it. Often we only update when there's a bug or significant improvement in the library.

I've tested, and bumping the version number gives us all the implemented chart types out of the box (which I think counts as a significant improvement).

This would of course be a major version bump, but I can't comment on the breaking changes for v10, it seems to work fine, but I don't have the perspective that regular contributors do. The breaking changes for mermaid v10 are listed here:
https://github.com/mermaid-js/mermaid/blob/master/CHANGELOG.md#1000

Fixes #8728, #8701, #6769

@github-actions
Copy link
Contributor

github-actions bot commented Sep 16, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@oj-lappi
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Sep 16, 2023
@oj-lappi
Copy link
Contributor Author

Related to #7629

@oj-lappi
Copy link
Contributor Author

Can confirm that mindmap charts work after this update as well.

@oj-lappi oj-lappi changed the title All: Resolves #87828: Bump mermaid version to 10.4.0 to support new chart types All: Resolves #8728: Bump mermaid version to 10.4.0 to support new chart types Sep 16, 2023
@oj-lappi
Copy link
Contributor Author

oj-lappi commented Sep 16, 2023

Fixes/resolves #8728, #8701, #6769

@wh201906
Copy link
Contributor

It seems the mermaid team has added the CJS support back, that's why bumping the version works
mermaid-js/mermaid@04305bd

@wh201906
Copy link
Contributor

@oj-lappi I found that @tessus changed more files than this PR when bumping the mermaid version. Would you please check the related files?
#5831 #6039 #6849 #7330

@oj-lappi
Copy link
Contributor Author

Ah, I had missed the postinstall step. I'll update tomorrow.

Is there a way to user test the mobile version on desktop btw? I've only tested the desktop app.

@tessus
Copy link
Collaborator

tessus commented Sep 16, 2023

Is there a way to user test the mobile version on desktop btw? I've only tested the desktop app.

Yep, https://github.com/laurent22/joplin/blob/dev/BUILD.md#testing-the-mobile-application

@jasonwilliams
Copy link
Contributor

jasonwilliams commented Sep 16, 2023

Great work @oj-lappi
Do you know if the update resolves any of the issues mentioned in the master issue?

#7629 (comment)

@oj-lappi oj-lappi force-pushed the joplin-feature-new-mermaid-charts branch from ee68811 to 010145a Compare September 17, 2023 09:01
 - 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
@oj-lappi oj-lappi force-pushed the joplin-feature-new-mermaid-charts branch from 010145a to dcb0417 Compare September 17, 2023 09:26
@oj-lappi
Copy link
Contributor Author

Just reran everything the way tessus outlined it, so now the mobile assets should all be there. I'll now try to test the mobile version, but I don't have the environment set up yet, so this may take a while.

@oj-lappi
Copy link
Contributor Author

oj-lappi commented Sep 17, 2023

Great work @oj-lappi Do you know if the update resolves any of the issues mentioned in the master issue?

#7629 (comment)

Yup, out of the documented issues, both are solved (#6769, #6711)

Of the undocumented issues, the ones below with checked boxes are resolved afaict (tested on desktop):

  • Mindmaps don't work
  • tooltips don't work
  • tooltips give me the title of the charts. On 9.2.2 some charts, like the flowchart, didn't recognize titles, but titles as tooltips work even on 9.2.2 for me for e.g. the Gantt chart or Pie chart
  • fa icons
  • fa icons show up in the rich text editor, but not in the markdown editor. In the rich text editor, the width of the fa icon is not used to calculate the width of the box its in, leading to part of the text being clipped off
  • support dark mode
  • the images drawn by mermaid still appear with a white background

@oj-lappi
Copy link
Contributor Author

oj-lappi commented Sep 17, 2023

Tested on a Pixel 7 AVD, all the charts tested render on android as well (timeline, mindmap, quadrant, Sankey). The font-awesome icons do not render on android, which makes sense since this was pretty much what I saw on desktop (fa icons only rendered in the rich text editor on desktop).

charts_0

image

Afaict, everything seems to work just fine.

@jasonwilliams
Copy link
Contributor

I've tested these changes (on desktop), I can see the mermaid diagrams are still working. I can also see:

  • Mindmap
  • Timeline
  • Sankey
  • Quadrant

all working fine.

I can see #6769 is fixed in this PR.
I can also see #6711 is fixed in this PR.

@laurent22 you will nee to approve the workflow so it can run.
I've approved these changes but @tessus may also want to look.

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

LGTM

@laurent22
Copy link
Owner

That's great, thanks guys for looking into this!

@laurent22 laurent22 merged commit e566f40 into laurent22:dev Sep 19, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mermaid quadrantChart (and experimental charts) are not working
5 participants