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

feature state diagram: add state diagram to markdown preview #3354

Merged
merged 1 commit into from Nov 21, 2019

Conversation

nammn
Copy link
Contributor

@nammn nammn commented Nov 12, 2019

Description

Bumped Mermaid version to support state diagrams.
Did sanity check whether mermaid is still working.

sequence diagram like before:
mermaid_sequence

new added state diagram:
mermaid_state


old description This PR uses the library: https://www.npmjs.com/package/state-machine-cat version 5.3.6 Do we want to add this dependency?

By adding this dependency we gain the ability to create state graphs by adding it as a state block in the markdown editor.

```state
initial => todo;
todo => doing;
doing => done;
done => final;
```

leads to this:
image

Without this we have the rely on the dependencies we are using right now, Mermaid and Plantuml.
Why they are not sufficient for this use-case can be found here further: #2933

Screengrab GIF

https://gfycat.com/marvelousexcitablekoala

speed up by 4x

Issue fixed

fixes #2933

Type of changes

  • ⚪ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • ❌ Feature (Change that adds new functionality)
  • ❌ Documentation change (Change that modifies documentation. Maybe typo fixes)
    -> I should update the example to contain a state diagram

Checklist:

  • ❌ My code follows the project code style
  • ❌ I have written test for my code and it has been tested
  • ❌ All existing tests have been passed
  • ❌ I have attached a screenshot/video to visualize my change if possible

IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@nammn nammn changed the title feature state: add state diagram to markdown preview WIP: feature state: add state diagram to markdown preview Nov 12, 2019
@nammn nammn changed the title WIP: feature state: add state diagram to markdown preview feature state diagram: add state diagram to markdown preview Nov 12, 2019
@nammn
Copy link
Contributor Author

nammn commented Nov 12, 2019

I just saw that the screengrab is cutting after 5 seconds. I will re-upload a new one tomorrow.

@Flexo013 Flexo013 added the awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. label Nov 13, 2019
@nammn
Copy link
Contributor Author

nammn commented Nov 13, 2019

@Flexo013 I added a link to a gif as the video was a bit longer than wanted.
You can speed up around 4-8x, it should only take ~2 seconds then :)

@Flexo013 Flexo013 added awaiting review ❇️ Pull request is awaiting a review. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Nov 13, 2019
@Flexo013
Copy link
Contributor

Nice! Could you add a simple image showing something that wasn't possible before this PR?

Also could you add a bit more in the description about what we gain by adding this dependency?

@nammn
Copy link
Contributor Author

nammn commented Nov 13, 2019

Ohh mermaid added support for state diagrams around 25 days ago in their 8.4.0 release.
I did not expect this. IMO they don't look good, but having fewer dependencies are always good. And they are used and supported well.

I could either:

What do you think? @Flexo013

@Flexo013
Copy link
Contributor

My gut feeling says that we should stick to mermaid, because fewer dependencies. Mermaid's version should be bumped regardless, so if that yields a working (and comparable) result to what we have in this PR, then I say we don't start using cat state.

@nammn
Copy link
Contributor Author

nammn commented Nov 13, 2019

Sure, I agree.
Let me take a look at this, probably either late today or tmw :)

@nammn
Copy link
Contributor Author

nammn commented Nov 13, 2019

@Flexo013 I reverted the code and "only" bumped mermaid version and its dependencies. Also did a sanity check whether things are still working. They do + we support state diagrams as well now :-)

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting review ❇️ Pull request is awaiting a review. labels Nov 14, 2019
@Rokt33r Rokt33r removed the approved 👍 Pull request has been approved by sufficient reviewers. label Nov 21, 2019
@Rokt33r Rokt33r added this to the v0.14.0 milestone Nov 21, 2019
@Rokt33r Rokt33r merged commit 9996b5d into BoostIO:master Nov 21, 2019
@nammn nammn deleted the add-state-diagram branch November 22, 2019 14:22
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.

Add State Diagram Support
4 participants