Adding mermaidJS for mermaid graphs #518
Conversation
|
Found 1 changed notebook. Review the changes at https://app.gitnotebooks.com/AnswerDotAI/fasthtml/pull/518 |
|
@ImtiazKhanDS Thanks for submitting this PR, I've added to my blog and given you credit: |
There was a problem hiding this comment.
@ImtiazKhanDS Thanks so much for this PR! This is something I think a lot of people will enjoy having. I do have a change request and a few comment.
Imtiaz's code can be seen in action here: https://daniel.feldroy.com/posts/mermaid-charts
| delay=500 # Delay in milliseconds before rendering | ||
| ): | ||
| "Implements browser-based Mermaid diagram rendering." | ||
| src = """ |
There was a problem hiding this comment.
This should be in a stand-alone JS file rather than a Python string template called with something like Script(rel='/mermaid.js').
There was a problem hiding this comment.
If you have a look at our other examples Danny we normally use ScriptX or python strings. Otherwise you have to deal with more complex file inclusion during deployment
There was a problem hiding this comment.
Apologies, I thought we had moved to a JS-file approach. Based on your response I just looked at https://github.com/AnswerDotAI/fasthtml-js and it wasn't what I thought it was.
fasthtml/js.py
Outdated
| proc_htmx('%s', () => { | ||
| setTimeout(renderMermaidDiagrams, %d); | ||
| }); | ||
| """ % (theme, sel, sel, delay) |
There was a problem hiding this comment.
While the JS in this file should be in its own file, I will recommend that 99% of the time f-strings are better than this kind of string variable injection. The reason is clarity, which makes debugging easier. The 1% edge case would be to increase performance, however in web dev strings are rarely the bottleneck.
There was a problem hiding this comment.
the benefit of py2 style formatting here is that you don't have to deal with doubling the curly brackets. it's a minor thing but can be more readable
fasthtml/js.py
Outdated
| } | ||
|
|
||
| proc_htmx('%s', () => { | ||
| setTimeout(renderMermaidDiagrams, %d); |
There was a problem hiding this comment.
Why the delay? I removed it from the code you submitted for my site, and it appears to work fine. I could be in error, if so we'll keep it in.
There was a problem hiding this comment.
Kept it to say to see the change happening , removed it now, should be working fine without that as well, thanks
There was a problem hiding this comment.
Afaict this isn't using proc_htmx correctly (although I might be missing something) - have a look at the proc_htmx source and make sure you understand its purpose and use.
There was a problem hiding this comment.
Thanks for the feedback @jph00 , proc_htmx already iterates with a selector and a js function. Changed it , code looks much more compact now.
|
|
Got it , did the changes. |
|
@ImtiazKhanDS there's no change to the .py file in your PR. I think you forgot to add |
Sorry for the trouble, now added. |
|
Good job! :D |
name: Pull Request for adding Mermaid Graphs
about: Propose changes to the codebase
title: 'Add Mermaid graphs in fasthtml websites.'
labels: 'graphs'
assignees: 'Jeremy'
Related Issue
Please link to the issue that this pull request addresses. If there isn't one, please create an issue first.
Proposed Changes
Describe the big picture of your changes here. If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:Checklist
Go over all the following points, and put an
xin all the boxes that apply:Additional Information
Any additional information, configuration or data that might be necessary to reproduce the issue.