inkdropapp / inkdrop-mermaid

Mermaid plugin for Inkdrop which lets you draw diagrams and flowcharts
https://www.inkdrop.app/
MIT License
27 stars 8 forks source link

feat(mermaid): Update mermaidjs to version 10.6.1 #18

Closed johmsalas closed 10 months ago

johmsalas commented 10 months ago

Solves #17

craftzdog commented 10 months ago

Hey, thanks for the PR! It works great. But with v10, diagrams randomly flicker when editing. Could you look into it?

https://github.com/inkdropapp/inkdrop-mermaid/assets/1332805/a8cb9678-9e83-47c1-bf73-840aaaad8a60

johmsalas commented 10 months ago

Rendering only the svg doesn't make it flick. My initial assumption, it is not the v10 of the library but the effort of rendering its svgs. I think a solution could be debouncing or throttling the input

wdyt @craftzdog?

{/* <div dangerouslySetInnerHTML={{ __html: this.state.svg }} /> */}
<div >
  {this.state.svg}
</div>

https://github.com/inkdropapp/inkdrop-mermaid/assets/6575405/7b955589-7f7e-4b8f-909f-49a7fed1b3a8

craftzdog commented 10 months ago

Rendering only the svg doesn't make it flick.

That's because Mermaid can't find the element associated with the this.mermaidId. I suspect that Mermaid removes the content of the div#{mermaidId} when rendering.

craftzdog commented 10 months ago

It doesn't flicker on Mermaid Live editor: https://mermaid.live/edit#pako:eNptkLFOxDAMhl_F57nwABkOHVQIZgaWLCYxNLo0LmkqqE737jhtByrw9Mv-P1u_L-jEMxoc-XPi5LgN9JGptwm0TjE4vjke7-XNwBPHKKCygU6-gDLDLBPcrVbtq3EBDDyGxA2UjtK5em7hlHwVm9dlpsIwUC7BhYFSgQfKcX-zdvRoWEaHHUiuSIYWaIRWEkW_TqtRwbZSG-B5LFnmP-u_1-2vvMQoItBTmveMJvon2jPof5gPvwsb7Dn3FLw-8lIhi6Xjni0alZ7y2aJNV_XRVORlTg5NyRM3OA1eE21PR_NOceTrD2bOf18

We might miss something 🤔

craftzdog commented 10 months ago

alright they set height: 100% 😂 https://github.com/mermaid-js/mermaid-live-editor/blob/1412bed15649000053f840f985c5bf332a71d09b/src/lib/components/View.svelte#L96

johmsalas commented 10 months ago

How does this sound? https://github.com/inkdropapp/inkdrop-mermaid/pull/18#discussion_r1442986641

craftzdog commented 10 months ago

found a workaround!

diff --git a/lib/mermaid.jsx b/lib/mermaid.jsx
index 1852799..8a3e969 100644
--- a/lib/mermaid.jsx
+++ b/lib/mermaid.jsx
@@ -10,10 +10,10 @@ export default class Mermaid extends React.Component {
         .toString(36)
         .replace(/[^a-z]+/g, '')
         .substr(0, 5)
-    this.state = { svg: '', error: null }
+    this.state = { error: null }
+    this.refContainer = React.createRef()
   }
   componentDidMount() {
-    this.renderDiagram()
     this.subs = inkdrop.config.observe('mermaid.theme', this.renderDiagram)
   }
   componentDidUpdate(prevProps) {
@@ -27,7 +27,6 @@ export default class Mermaid extends React.Component {
   shouldComponentUpdate(nextProps, nextState) {
     return (
       nextProps.children[0] !== this.props.children[0] ||
-      nextState.svg !== this.state.svg ||
       nextState.error !== this.state.error
     )
   }
@@ -42,7 +41,7 @@ export default class Mermaid extends React.Component {
           autoScale ? '' : 'disable-auto-scale'
         }`}
       >
-        <div dangerouslySetInnerHTML={{ __html: this.state.svg }} />
+        <div ref={this.refContainer} />
         {error && (
           <div className="ui error message">
             <div className="header">Failed to render Mermaid</div>
@@ -59,11 +58,13 @@ export default class Mermaid extends React.Component {
     const [code] = children || []
     try {
       if (typeof code === 'string') {
+        const elContainer = this.refContainer.current
         const { svg } = await mermaid.render(this.mermaidId, code)
-        this.setState({ svg, error: null })
+        elContainer.innerHTML = svg
+        this.setState({ error: null })
       }
     } catch (e) {
-      this.setState({ error: e, svg: '' })
+      this.setState({ error: e })
     }
   }
 }

Instead of rendering svg with state, simply setting innerHTML avoids the flickering. Can you try it?

johmsalas commented 10 months ago

Hi @craftzdog!!, thanks for keeping me in the loop. It sounds like it is now under your control. And tbh this is more involved than expected. I was able to create the charts I required already in my local setup, I'm going to move away from the solution and let you finish it, because you'll also want to be really careful for not breaking your App

Definitely looking forward to your submission of this solution so that I can open the charts in my secondary computer. Appreciate the good work you've done with InkDrop.

Just some concerns before you keep on, innerHTML is not tracked by React reconciliation algorithm and it might be override if the props changed, not to worry, just take it into account in the component workflow

Once again, thanks for the great app

craftzdog commented 10 months ago

yeah it works great so far. printing also works properly. thanks for the contribution! I really appreciate it.

craftzdog commented 9 months ago

landed in 2.7.0!