Skip to content

Conversation

evnchn
Copy link
Collaborator

@evnchn evnchn commented Jun 12, 2025

Motivation

@thetableman, I feel sorry for pushing back against #4845, so I spent about 2 hours to code what I think could work to let Mermaid have node click functionality, with a minimally-sketchy code change. Notably, I used string format instead of regex, so the change NiceGUI is making should be obvious and understandable to everyone.

Implementation

If we desire to add on_node_click for Mermaid:

  • Setup function mermaid_click_handler_SOMEUUID (since this is not available), .
  • Instead of using regex, which are less predictable and a bit of a black box, I use string format (formatting against {handler}) and replace that with the actual function name

Shows up clearly in the IDE as well:

{562E07C9-AB7D-46E6-8BD8-D01AB95B5D1D}

Progress

  • I chose a meaningful title that completes the sentence: "If applied, this PR will..."
  • The implementation is complete.
  • Pytests have been added (or are not necessary).
  • Documentation has been added (or is not necessary).

Pytest pending. Documentation pending. I'm tired TBH.

Test script

from nicegui import ui


@ui.page('/')
def main_page():
    ui.mermaid('''
        graph LR;
            A --> B;
            B --> C;
            click A {handler}
            click B call {handler}()
            click C call {handler}("C", "C some args")
        ''', on_node_click=lambda e: ui.notify(e))


ui.run()

Result

{BA0717C3-3735-48B0-9FC0-2ACCE953FEA3}

@evnchn
Copy link
Collaborator Author

evnchn commented Jun 12, 2025

@falkoschindler I forgot how to do custom event arguments 😅, so it is GenericEventArguments as it is. I expect in the future that it's something like:

@dataclass(**KWONLY_SLOTS)
class JsonEditorChangeEventArguments(UiEventArguments):
    node: str
    param: Any

Of course, the user has to do due diligence and check the node whether it is valid node, since if they do click C call {handler}("blah", "C some args"), then they record to the backend that node blah was clicked, which doesn't exist in the first place, and is likely going to cause some errors down the line.

@thetableman
Copy link
Contributor

@evnchn, thanks for taking this on.

What's the outcome if that mermaid markdown is created using an f-string, won't that {handler} placeholder become an issue?

@evnchn
Copy link
Collaborator Author

evnchn commented Jun 12, 2025

Supposedly, they need to ensure that their f-string, after substitution, gets to have a {handler} in them

Could be some very nasty syntax to get that working. Let me assess.

@evnchn
Copy link
Collaborator Author

evnchn commented Jun 12, 2025

Ok so apparently for f-string, you need {{handler}} so that after f-string does its substitution, it becomes {handler} and the format in our ui.mermaid works.

Syntax is a bit nasty, but i think it's better than a direct replace (cannot escape the keyword if keyword turns out to be inside Mermaid diagram) / regex (hard to grasp)

@falkoschindler
Copy link
Contributor

Thanks for the proposal, @evnchn!

I'm still sceptical. Consider the following diagram:

ui.mermaid('''
    graph LR
        A[trigger] -->|event| B{handler}
''')
Screenshot 2025-06-12 at 17 11 42

The diamond shape looks like our new event handler, but is normal Mermaid syntax. If we want to extend the Mermaid language with a custom event handler notation, we need to find something that isn't already part of the language. This could, however, get pretty cryptic.

@falkoschindler falkoschindler added feature Type/scope: New feature or enhancement analysis Status: Requires team/community input labels Jun 12, 2025
@evnchn
Copy link
Collaborator Author

evnchn commented Jun 12, 2025

@falkoschindler Can agree that the Python f-string is getting cryptic. I also pondered the possibility that someone would invoke the browser-side Mermaid content update function by means of getElement(5).update, which this PR would be incompatible with, since the find-and-replace logic is at Python-side.

With that, I made #4862, the diff is smaller, and I don't think it would be possible to run into collision issues unlike this PR.

Marking this as draft, unless you want to switch gears back to this approach, which I think is rather unlikely.

@evnchn evnchn marked this pull request as draft June 12, 2025 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analysis Status: Requires team/community input feature Type/scope: New feature or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants