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

Directives slight rework, utils enhancements, mermaidAPI enhancements, SequenceDiagram tweaks #1482

Merged
merged 8 commits into from Jun 19, 2020
Merged

Conversation

chrismoran-bkt
Copy link
Contributor

📑 Summary

Slight refactor for sequenceDiagram db/renderer

  • Fixed default configuration woes introduced by yours-truly with the directives feature (built on fixes Knut added for this same issue)
  • Added freeze to mermaidAPI to protect a new exported defaultConfig
  • Enhanced reset to revert any modifications to the current config back to the defaultConfig
  • Enhanced initialize to merge user config into mermaidAPI current config without losing the defaultConfig
  • Enhanced initialize to update each renderer config
  • Enhanced init/initialize directive to support a universal 'config' property which will be changed (based on detectType) to the proper config-object-name for the graph type in scope. Example:
%%{init: { 'config': {'wrapEnabled': true }}}%%
sequenceDiagram
participant Alice

will correctly set the config property of the initialization object to sequence effectively resulting in the following object to be sent to mermaidAPI.initialize()

{
  'sequence': {
    'wrapEnabled': true
  }
}

which, when combined with the new assignWithDepth functionality will properly update the current config of the mermaidAPI (without losing other defaults) as well as updating the sequenceRenderer config

  • Directives have been reworked a little
    • Now any graph type can include a %%{init: { 'config': {..}}}%% directive and their config will correctly be converted to the properly-named config for the graph they are describing without losing the default config in the process.
    • User configuration is now overlaid properly on top of default config
  • Moved a fair number of common functions to utils.js
    • wrapLabel - breaks a label(text) into a '
      ' delimited string for wrapping long strings
    • calculateTextDimensions - calculate the width, height of a block of text (taking into account linebreaks)
    • drawSimpleText - function lifted from sequence/svgDraw.js for drawing a simple text/tspan via svg (used primarily for calculating text dimensions)
    • drawText - lifted from sequenceRenderer and modified slightly to handle text with or without linebreaks and render the text properly.
    • assignWithDepth - new function to expand on Object.assign for use with merging config (taking into account arbitrary object graph depth)
  • Enhanced sequenceDiagram rendering to support wrapping on loop types
    • The loop/opt/alt/par description text now wraps properly (if wrapEnabled is true)
  • Fixed an odd issue with sequence diagrams where useMaxWidth was not properly using the max width
  • Many new unit tests for all of the above

If i missed a test please let me know. I tried to be thorough.

Resolves #1473

chrismoran-bkt and others added 8 commits June 14, 2020 14:14
# Conflicts:
#	dist/mermaid.core.js
#	dist/mermaid.core.js.map
#	dist/mermaid.js
#	dist/mermaid.js.map
#	dist/mermaid.min.js
#	dist/mermaid.min.js.map
#	src/config.js
#	src/mermaidAPI.js
Fixed default config clobbering issues
Fixed default config clobbering issues
Updated/corrected sequenceDiagram.spec to set the config using mermaidAPI
Enabled freeze on mermaidAPI to protect defaultConfig
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.

Lots of goodies in here. Code wise it is good. The config refactoring has been overdue for a while so it is good that it is happening.

As a reviewer it is easier to review several smaller PRs then one large which also make writing the release notes easier.

Also good with the new tests, both the unit tests and rendering tests.

I noticed this change:

diagram.attr('style', 'max-width:' + width + 'px;');

to

diagram.attr('style', 'max-width:100%;');

This will change the behaviour of the useMaxWidth so that small diagrams become larger. The old version keeps the small diagram in the original size but changes the size of diagrams that are too large. I will change that back.

Also I noticed the return parser in mermaidAPI.parse, thats a good idea. We could perhaps have only one switch case for the the diagram type resulting in a parser and a renderer etc. Lots of text improvements for sequenceDiagrams, specifically it was good to break out the text functions from sequence diagram as they are usefull for other diagrams for instance in the new dagre-wrapper.

Thanks for a thurough job!

@knsv knsv merged commit 24ed979 into mermaid-js:develop Jun 19, 2020
@knsv
Copy link
Collaborator

knsv commented Jun 19, 2020

Percy found some unintended side effects:
https://percy.io/Mermaid/mermaid/builds/5754554

For instance from:
image
to:
image

@knsv
Copy link
Collaborator

knsv commented Jun 19, 2020

The wrap concept is elegant but when it is not present it needs to be rendered in a good way by default. If we release this as is, there would be diagrams out there, authored by end users, breaking with the new version.

Ideally eitherPerhaps wrapping could be default behavior?

I registered an issue for this: #1483

@knsv
Copy link
Collaborator

knsv commented Jun 19, 2020

Another issue in noticed thanks to the new test you introduced (👍). I think this was not rendering well before either. Now it looks like it being wrapped by the y value is not increased.

image

knsv added a commit that referenced this pull request Jun 19, 2020
@knsv
Copy link
Collaborator

knsv commented Jun 19, 2020

@chrismoran-bkt I really hope you take the time and effort to make a PR for this as it would be a waste not to get these changes in.

I have moved the changes from the PR into a feature branch to avoid having next release depending on a pontential PR.
https://github.com/mermaid-js/mermaid/tree/feature/1483_long_messages

Also if you make a PR point it to the feature branch.

@chrismoran-bkt
Copy link
Contributor Author

chrismoran-bkt commented Jun 19, 2020 via email

@chrismoran-bkt
Copy link
Contributor Author

Let me know what you think of #1484

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.

Default config is lost after directives update
2 participants