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

Updated version of mermaid.js #160

Merged
merged 1 commit into from
Feb 4, 2016
Merged

Updated version of mermaid.js #160

merged 1 commit into from
Feb 4, 2016

Conversation

vnijs
Copy link
Contributor

@vnijs vnijs commented Feb 4, 2016

Fixes rendering problems in Chrome (mermaid-js/mermaid#281) and color for circles (mermaid-js/mermaid#271).

I talked to @ramnathv shiny-conference in SF this past weekend. He suggested the pull request. Also cc-ing @timelyportfolio

rich-iannone added a commit that referenced this pull request Feb 4, 2016
Updated version of mermaid.js
@rich-iannone rich-iannone merged commit 9237df7 into rich-iannone:master Feb 4, 2016
@rich-iannone
Copy link
Owner

@vnijs Thanks very much! I always appreciate PRs and I accept them without hesitation about 99% of the time (and this is a good one).

@vnijs
Copy link
Contributor Author

vnijs commented Feb 4, 2016

@rich-iannone Great! The one thing you may want to double-check is the mermaid.css file. The previous version had .mermaid in front of most entries. I did try that but then circle coloring broke again.

@timelyportfolio
Copy link
Contributor

@vnijs I really appreciate you doing this. I have lost touch a little with mermaid. The lack of .mermaid namespacing very likely will conflict if other htmlwidgets are on the same page, since many very common classes like .node are used. I'll try to take a look to see if I can get it namespaced again. However, it might not be possible without changes to the mermaid.js code, which I'm not all the willing to implement.

@timelyportfolio
Copy link
Contributor

@vnijs, was there a particular example/test that failed, so I can insure that my changes work or fail?

@timelyportfolio
Copy link
Contributor

It is what I remember a battle of specificity, so will require changes to mermaid source. In general I shy away from manipulating the source since maintenance then becomes very troublesome.

@vnijs
Copy link
Contributor Author

vnijs commented Feb 4, 2016

@timelyportfolio

Mermaid plots currently don't render at all (at least in Chrome). Here is an example and how it should look. This works with the PR.

screen shot 2016-02-04 at 11 52 53 am

graph LR
id1[$1,569.22] --- |No research| id2[$1,400]
id1[$1,569.22] === |Research| id7(($1,569.22))
id2 === |Introduce| id3(($1,400))
id2 --- |Don't introduce| id6[$0]
id7 --- |Positive recommendation: 0.62| id8[$2,531]
id7 --- |Negative recommendation: 0.38| id13[$0]
id3 --- |Success: 0.6| id4[$4,000]
id3 --- |Failure: 0.4| id5[$-2,500]
id8 === |Introduce| id9(($2,531))
id8 --- |Don't introduce| id12[$0]
id13 --- |Introduce| id14(($-446))
id13 === |Don't introduce| id17[$0]
id9 --- |Success: 0.774| id10[$4,000]
id9 --- |Failure: 0.226| id11[$-2,500]
id14 --- |Success: 0.316| id15[$4,000]
id14 --- |Failure: 0.684| id16[$-2,500]
classDef default fill:none, bg:none, stroke-width:0px;
classDef chance fill:#FF8C00,stroke:#333,stroke-width:1px;
classDef decision fill:#9ACD32,stroke:#333,stroke-width:1px;
class id1,id2,id8,id13 decision;
class id3,id7,id9,id14 chance;

@gluc
Copy link
Contributor

gluc commented Feb 13, 2016

@vnijs Now, this is far from being beautiful, and it doesn't have the angled edges as you like them, and it's also less complex. But it renders fine. It's in from the latest data.tree release's application vignette. The reason I replaced the Mermaid code is that a few weeks ago, I had problems with the arrow heads. And since I also added a generic plot functionality to data.tree (based on DiagrammeR grViz), I figured this would make sense.

dtree

@vnijs
Copy link
Contributor Author

vnijs commented Mar 27, 2016

Thanks for sharing the plot @gluc. However, I much prefer mermaid for these plots, mainly because of how edge-labels are added.

@timelyportfolio's point is an important one of course. However, mermaid plots no longer work in Chrome with the old version of mermaid.slim.min.js that was part DiagrammeR.

I noticed that @achubaty put back the .mermaid entries in the CSS once already but it isn't clear to me what changes would be needed in the source code to use this CSS structure appropriately. I'm happy to help but could use a few pointers to get started.

In addition to possible conflicts it seems that with the github version of knitr, mermaid plots are rendered incorrectly in R-studio with the github version of mermaid.slim.min.js and mermaid.css in DiagrammeR. See example below. Although I don't think this is a knitr issue, it is odd that if you remove the A header head the graph is rendered correctly.

cc @yihui

---
title: "Test"
output: html_document
---

## A header

Note that if the previous header is removed the rendered HTML looks fine.

```{r echo}
library(DiagrammeR)
mermaid("
graph LR
  A-->B
  A-->C
  C-->E
  B-->D
  C-->D
  D-->F
  E-->F
")

image

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.

4 participants