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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/sequence diagrams improvements #1365

Merged

Conversation

dany74q
Copy link
Contributor

@dany74q dany74q commented Apr 23, 2020

馃搼 Summary

This commit fixes some bugs, and I believe, improves upon the current
implementation.

Resolves #494

馃搹 Design Decisions

Mermaid is awesome - I use it a lot (it's even more awesome w/ emacs & ob-mermaid);
and that's why I think that we should make a few improvements to the sequence diagram rendering.

I think that it falls short, compared to other diagram types, in that it renders rectangles with a fixed size, as opposed to dynamically scaling them when, for instance, the text is larger than the container.

There are workaround, of course - one can set a larger width for the rectangles - that yield an awkward result though, b/c all rectangles are rendered with the same width;
and we'd probably like to have the ability to have a sole rectangle that should be bigger, and others that should be smaller.

The following is a list of proposed fixes that I believe would make the usage of sequence diagrams better:

  1. Control over note font size, family and alignment (now defaults to
    center)
  2. Dynamic actor resizing - actor's width will now scale if its
    description is bigger than the static configured width
  3. Dynamic actor margins - the margin between actors will now be
    dynamically calculated by taking into account the width of connecting
    messages or notes
  4. Fixed a small visual annoyance where a loop arrow would intersect
    with the text it loops on
  5. Fixed a bug where if global config -> fontFamily wasn't defined, it
    would override the actorFontFamily with an undefined
  6. Removed some stale / commented out code
  7. Added missing config variables to the global config object in mermaidAPI.js
  8. Fixed doc strings for methods which had an outdated one
  9. Added e2e tests for the above scenarios
  10. Added documentation for the new config keys
  11. Linted the sequence diagram markdown doc
  12. Refactored a bit where necessary

All tests pass - and looking at the generated dist/index.html, with some additional use cases, looks pretty good.

馃搵 Tasks

Make sure you

  • [ /] 馃摉 have read the contribution guidelines
  • [ /] 馃捇 have added unit/e2e tests (if appropriate)
  • [ /] 馃敄 targeted develop branch

This commit fixes some outdated docstrings, replacing the description
and @params where necessary.
This commit extract the commonly used /br\s*\/?>/gi regex to common.js,
in order to keep the code more DRY.
Copy link
Collaborator

@knsv knsv left a comment

Choose a reason for hiding this comment

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

Hi! Good stuff!

This looks neat and we should add this to mermaid. One issue is that by bypassing the template this causes extra administration and increasing the risk that this is missed in the release notes. Could you please add that? This should not take to long.

Next thing, could you also add those diagrams you mentioned to rendering tests? Its quite easy, look in cypress/integration/rendering/sequenceDiagram.spec.js

Here is a sample test:

    it('should render autonumber when autonumber keyword is used', () => {
      imgSnapshotTest(
        `
        sequenceDiagram
        autonumber
        Alice->>John: Hello John, how are you?
        loop Healthcheck
            John->>John: Fight against hypochondria
        end
        Note right of John: Rational thoughts!
        John-->>Alice: Great!
        John->>Bob: How about you?
        Bob-->>John: Jolly good!
      `,
      {}
      );
    });

Just add new ones with diagram showing this new feature.

This helps testing, making sure it wont brea by some other change in the future.

Looking forward to merging this.

This commit fixes some bugs, and I believe, improves upon the current
implementation.

In no particular order, it adds:

1. Control over note font size, family and alignment (now defaults to
center)
2. Dynamic actor resizing - actor's width will now scale if its
description is bigger than the static configured width
3. Dynamic actor margins - the margin between actors will now be
dynamically calculated by taking into account the width of connecting
messages or notes
4. Fixed a small visual annoyance where a loop arrow would intersect
with the text it loops on
5. Fixed a bug where if global config -> fontFamily wasn't defined, it
would override the actorFontFamily with an undefined
6. Removed some stale / commented out code
7. Added missing config variables to the global config object in mermaidAPI.js
8. Added messageFontSize, messageFontFamily to control message (non-note)
font settings
9. Memoized the actor widths in a pre-calculation that takes notes and
signals lengths into account
10. Removed redundant console.log lines
11. Extracted out actor width & margin calculation to getMaxMessageWidthPerActor, and
calculateActorMargins
@dany74q dany74q force-pushed the feature/sequence-diagrams-improvements branch from 60d78ec to 5f6887b Compare April 23, 2020 13:54
This commits adds e2e tests for the suggested improvements.
I've went over the generated screenshots and they look good to me.
This commit adds documentation to new config variables that were
introduced as part of this PR, namely, the font settings for actors,
messages and notes.

I've also linted the markdown document.
@dany74q dany74q requested a review from knsv April 23, 2020 15:45
@dany74q
Copy link
Contributor Author

dany74q commented Apr 23, 2020

Hey hey @knsv - thanks for the feedback, I've revisit the PR, added e2e tests
and updated the documentation.

Feel free to go over it (For a quick diff, you can add a cy.screenshot() to cypress, and compare the added tests between the versions) -
I think the results are pretty cool.

Let me know what you think,
Danny

Copy link
Collaborator

@knsv knsv left a comment

Choose a reason for hiding this comment

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

Thanks! The extra effort is much appreciated!

@knsv knsv merged commit 3aa5fc0 into mermaid-js:develop Apr 26, 2020
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.

MermaidAPI text overflow out of the node
2 participants