-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Integrated Katex typesetting into flowcharts #2885
Integrated Katex typesetting into flowcharts #2885
Conversation
…feature/2776_katex_math
Hi Nicolas! Well done, that looks spectacular! I am eager to merge this PR but I am a bit concerned about the font part and the consequences in mermaid. I will need to evaluate this in more depth to make sure this does not cause issues for projects downstream. Thanks again! |
FYI @NicolasNewman Reviewing this PR is not forgotten but a little delayed, due to easter time-crunch. It will happen next week. |
No problem. Thank you for the heads up! |
I have started to look at this PR and again I must say it looks great! The main problem is that the size of mermaid grows quite a lot with these changes, 330k minimized (not gzipped), this is an increase of almost 30% which is too much. We are already struggling with mermaids weight and are trying to decrease it. Pulling out the fonts and the katex css is one thing that could be done but I suspect it will only go so far. There is nothing wrong with your code, I find the font handling elegant but the size increase is a showstopper. (Out of curiosity, why do you need the sass-loader when you are only loading css files?) I don't want to give up just yet though. There is an item in the roadmap to add lazy loading into mermaid. I think we should bump up the priority of lazy loading and make this lazy loaded, alternatively having a different bundle with this mermaid-katex.min.js. I like the lazy loading better though. |
The size is an unfortunate side effect of math typesetters due to the large number of characters they add. Until MathML gets implemented in every browser there's unfortunately not much that can be done to reduce Katex's size. Sadly other then Firefox, MathML is very low on vendor's backlog. Sass-loader should be able to be removed. I'm not as familiar with Webpack so while I was troubleshooting why the CSS file wasn't getting included, I tried adding sass-loader. I noticed it was being referenced in the config but not installed as a dependency. If there's no other reason for it to be installed, I should be able to remove it. I'm assuming it should still be referenced in the config file as before? In terms of solutions, I agree with lazy loading being the ideal approach. Depending on how long that could take, do you think it would be worth while to have a separate bundle in the meantime? |
yes, we could add an experimental bundle. while the regular bundles stay the same |
Is there anything I would need to do on my end to make that happen? |
Any update on this item? |
MathML was very recently added into Chromium which means we don't need to rely on Katex's Stylesheet. I plan on fixing merge conflicts later this week. Few Chrome and Edge users have updated though so it will still take time until we can expect most users to be using a browser with built-in support. I go into more detail about it in #2776 Unfortunately we still won't have MathML support for Opera, Samsung Internet, UC, or IE so a discussion will need to be had on how to support potential users on those browsers. |
The fix was minor, but it took a lot of time to figure it out. If this file had originally been typed fully, this error wouldn't have occurred at all. This is a prime example of why we should focus some more on TS conversion, and ensure new files added are fully typed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, the journey has not been short but I think we are ready at last!
This is a great feature and I look forward to merging it.
fe1cff3
@NicolasNewman, Thank you for the contribution! |
I've recently graduated and started working so things have been very busy but I'm glad to see this finally be merged in! It was certainly quite the journey. I greatly appreciate everyone's support on making this feature a reality over the past two years. I'll keep on eye on requests for additional support in other charts but I won't have as much time to spend on it as I once did. |
Do you know when it comes available into GitHub markdown syntax: graph LR
A["$$f(\relax{x}) = \int_{-\infty}^\infty \hat{f}(\xi)\,e^{2 \pi i \xi x}\,d\xi$$"] -->|"$$\Bigg(\bigg(\Big(\big((\frac{-b\pm\sqrt{b^2-4ac}}{2a})\big)\Big)\bigg)\Bigg)$$"| B("$$1+\frac{e^{-2\pi}} {1+\frac{e^{-4\pi}} {1+\frac{e^{-6\pi}} {1+\frac{e^{-8\pi}} {1+\cdots}}}}$$")
A -->|"$$\overbrace{a+b+c}^{\text{note}}$$"| C("$$\phase{-78^\circ}$$")
B --> D("$$x = \begin{cases} a &\text{if } b \\ c &\text{if } d \end{cases}$$")
C --> E("$$x(t)=c_1\begin{bmatrix}-\cos{t}+\sin{t}\\ 2\cos{t} \end{bmatrix}e^{2t}$$")
current gh version seems to be: info
|
There hasn't been a new Mermaid release since this feature has been merged. The Katex feature has been merged on 13 February 2024, and the current latest release of Mermaid (v10.8.0) was on 2 February 2024. Once a new Mermaid release has been made, we'll have to wait for GitHub to update its Mermaid version. Hopefully that won't be too long! |
Thank you for your great work. This closes a gap that existed for so long. |
ok, it was released on v10.9.0 Github is currently info
Let's wait GitHub to update, so this can be properly rendered: graph LR
A["$$f(\relax{x}) = \int_{-\infty}^\infty \hat{f}(\xi)\,e^{2 \pi i \xi x}\,d\xi$$"] -->|"$$\Bigg(\bigg(\Big(\big((\frac{-b\pm\sqrt{b^2-4ac}}{2a})\big)\Big)\bigg)\Bigg)$$"| B("$$1+\frac{e^{-2\pi}} {1+\frac{e^{-4\pi}} {1+\frac{e^{-6\pi}} {1+\frac{e^{-8\pi}} {1+\cdots}}}}$$")
A -->|"$$\overbrace{a+b+c}^{\text{note}}$$"| C("$$\phase{-78^\circ}$$")
B --> D("$$x = \begin{cases} a &\text{if } b \\ c &\text{if } d \end{cases}$$")
C --> E("$$x(t)=c_1\begin{bmatrix}-\cos{t}+\sin{t}\\ 2\cos{t} \end{bmatrix}e^{2t}$$")
|
That's unfortunate. I'll start looking into what's going on. |
Bug report opened: #5482 |
📑 Summary
This PR integrates the Katex math typesetter into flowcharts(v2), allowing for complex mathematical formulas to be typed inside of charts
Resolves #2776
📷 Example
📏 Design Decisions
🥅 Future Goals
📋 Tasks
Make sure you
develop
branch