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

Use mermaid for "defmt ecosystem" diagram #764

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Use mermaid for "defmt ecosystem" diagram #764

wants to merge 6 commits into from

Conversation

Urhengulas
Copy link
Member

What

This PR removes the previous (static) diagram of the defmt ecosystem from the README and replaces it with a mermaid code block.

Context

Mermaid is a JavaScript based diagramming and charting tool that renders Markdown-inspired text definitions to create and modify diagrams dynamically. It is widely used. By now both GitHub and crates.io do render mermaid diagrams, in GitHub you can even interact with them (zooming in and out mostly, but still).

Why

This change will make it easier to keep the diagram up to date, since we don't rely on other tooling (draw.io in our case) to modify the diagram.

Drawbacks

As a grain of salt I have to say that I actually do like the aesthetics of the previous diagram more (see below). But I still think that the advantages outweigh that. It is possible to style the mermaid diagram with CSS, but I did not want to go down that road.

Before

defmt

After

mermaid-diagram-2023-07-07-142929

Remove md styling, as github doesn't render that

Remove legend

Add legend as separate graph

Remove titles, doesn;t render

= for pinned dependencies

Add legend with invisble link

The link (hopefully!) fixes it's positioning
@Urhengulas Urhengulas requested a review from justahero July 7, 2023 12:32
@Urhengulas
Copy link
Member Author

I will do a pre-release on crates.io to see if it renders there as well.

@Urhengulas
Copy link
Member Author

I will do a pre-release on crates.io to see if it renders there as well.

Unfortunately it fails to render. See https://crates.io/crates/defmt/0.3.6-beta.0

crates.io shows a warning popup with "Failed to render mermaid diagram." and in the console it prints "WARN: Could not find the language 'mermaid', did you forget to load/include a language module?".

It could be because I am not using the default renderer.

@Urhengulas
Copy link
Member Author

It could be because I am not using the default renderer.

Nope. Changing back to the default renderer does not change anything.

@tshepang
Copy link

tshepang commented Jul 7, 2023

As a grain of salt I have to say that I actually do like the aesthetics of the previous diagram more (see below).

hm, I find the mermaid one (at least as rendered here) more pretty

README.md Show resolved Hide resolved
@Urhengulas
Copy link
Member Author

We need to wait for rust-lang/crates.io#6786 to be resolved

@Urhengulas
Copy link
Member Author

We need to wait for rust-lang/crates.io#6786 to be resolved

The bug is fixed and does render on crates.io now! 🎉

auto-merge was automatically disabled August 1, 2023 10:14

Merge queue setting changed

@@ -29,7 +29,40 @@ It still might work on older rust versions, but this isn't ensured.

The following diagram illustrates the user-facing and internal crates of the defmt framework.

![defmt crates structure](assets/defmt.png)
```mermaid
%%{ init: { 'flowchart': { 'curve': 'step', "defaultRenderer": "elk" } } }%%
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"defaultRenderer": "elk" seems to be the culprit for the ~~~ invisible link not working.

Copy link
Contributor

@BriocheBerlin BriocheBerlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my mind the mermaid chart is less clear than the old one because the crossed out links were more obvious. This makes the legend being connected even more confusing.
Not using the elk renderer produces this
Screenshot 2023-08-03 at 15 58 26
Also not great.
Maybe do ask the mermaid people for the invisible link with the elk renderer?

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.

None yet

3 participants