feat(flowchart): add classDef style group definition

[tomperr]

📑 Summary

Enable to add styles to many classDef in a single statement
Add unit test on addClass function of flowDb

Resolves #3036

📏 Design Decisions

Split the ids using ,.
e.g. classDef a,b stroke:gold

📋 Tasks

Make sure you

• 📖 have read the contribution guidelines
• 💻 have added unit/e2e tests (if appropriate)
• 🔖 targeted develop branch

[weedySeaDragon]

I have some questions about this PR:

1. What is the purpose? I'm not clear on this
2. Your code doesn't change anything for the user

1. Purpose?

Can you clarify what the goal of this PR is? I've read through issue #3036.
I see your comment that a user can indeed apply a style definition (classDef) to one or many nodes or links. (Here's the documentation section on that.)
But I'm not clear on why you want to define a style class (classDef = style class definition) with multiple ids (names).

For example, here is the statement that a users would (theoretically) write in a flowchart diagram text in order to call the addClass() method with many class ids (names):

classDef firstStyleID,secondStyleID, font-size: larger, color:red

firstShape[First shape] --> secondShape(second shape)

%% Here is where the style (with 2 different ids) could be applied:
class firstShape firstStyleID
class secondShape secondStyleID

This would mean you are defining the same class style (font is larger and color is red) with 2 different names: firstStyleID and secondStyleID

A user would (and should) just use firstStyleID everywhere in their diagram definition, like this:

class firstShape,secondShape firstStyleID

I may very well be missing something.

2. No change for a user

Your code does change how flowDb addClass() can work (it can accept multiple class ids for a single class style definition). (And kudos for writing tests!!) But it doesn't change what can be parsed from the user.
You'd need to also change the parser in flowchart/parser/flow.jison (and write tests, of course.)

[tomperr]

Hey, thanks for your review.
I may have missed something, but if I'm not wrong, it is currently not possible to add style to many classes in one statement (which is something common in CSS for example). I will take a look again in the next days. Sorry for this unclear PR, I will add new comments once checked a second time to make it clear

[weedySeaDragon]

Thanks for clarifying for me. Makes more sense now.
(You'll still need to modify the parser flow.jison. But I think you understand that. :-) )

[tomperr]

Sorry for the wait. I will start from the beginning so that everything is clear (I hope :))

Purpose

Initially, this issue was about declaring style (with linkStyle) for many links at the same time, instead of having one statement for each link which could be redundant:

linkStyle 0,1,2 stroke:gold, stroke-width:4px

instead of

linkStyle 0 stroke:gold, stroke-width:4px
linkStyle 1 stroke:gold, stroke-width:4px
linkStyle 2 stroke:gold, stroke-width:4px

---

In the discussion, people thought that it could be great to have the same for classes: being allowed to add a class to many nodes at the same time:

class A,B,C active_node;

instead of

class A active_node;
class B active_node;
class C active_node;

---

I discovered that those two features were already implemented. But to be consistent, why not having the same for classDef? classDef add style to a class, and to avoid redundancy in some cases, declaring styles to many classes at the same time could be interesting:

classDef basic_nodes,error_nodes,success_nodes stroke-width:8px,font-size:12pt,color:white

classDef basic_nodes fill:#A0A0A0,stroke:#A0A0A0
classDef error_nodes fill:#BA1919,stroke:#BA1919
classDef success_nodes fill:#2F9A11,stroke:#2F9A11

instead of

classDef basic_nodes fill:#C0C0C0,stroke:#C0C0C0,stroke-width:8px,font-size:12pt,color:white
classDef error_nodes fill:#BA1919,stroke:#BA1919,stroke-width:8px,font-size:12pt,color:white
classDef success_nodes fill:#2F9A11,stroke:#2F9A11,stroke-width:8px,font-size:12pt,color:white

No change for a user

Let's take the example of the following statement:

classDef basic_nodes,error_nodes,success_nodes stroke-width:8px,font-size:12pt,color:white

Currently (on the develop branch), once this statement is parsed, the addClass function will be called with "basic_nodes,error_nodes,success_nodes" as id parameter, and "stroke-width:8px,font-size:12pt,color:white" as style parameter. So basic_nodes,error_nodes,success_nodes is treated as one class, and not the concatenation of many. For this reason, we have to split it (using comma separator) and apply styles to each of them.

Modifying the parser is not (in my opinion) mandatory, as classes and styles are well detected. Instead of having the string "basic_nodes,error_nodes,success_nodes" in id parameter of the addClass function, we may have an array like ["basic_nodes", "error_nodes", "success_nodes"] which would be easier to use. However, code that allows group class definition (explained in the previous section) doesn't implement it this way:

classStatement:CLASS SPACE alphaNum SPACE alphaNum
    {$$ = $1;yy.setClass($3, $5);}
    ;

export const setClass = function (ids, className) {
  ids.split(',').forEach(function (_id) {
    // ...
  });
};

This could be a good refactoring, but the behavior won't change, and to be honest I'm still not comfortable with Bison grammar rules.

[tomperr]

I will add more documentation on all the features I talked about, and I will try to add more tests :)

[brunolnetto]

Hi @tomperr. It has been a while since your last comment here. Are you ok? If yes, it would be kind of you if you made any advance on this direction e.g. on these classDef incremental properties on mermaid-code-string composition. I am particularly interested on graphs with colorful arrows and classDef groups. :-)

[tomperr]

Sorry, I just don't have the time these days

[brunolnetto]

No worries. Take your time.

[jgreywolf]

@tomperr I have tagged @knsv and @sidharthv96 to review your PR as is

[jgreywolf]

@knsv @sidharthv96 Can you please review?

[codecov-commenter]

Codecov Report

Merging #3923 (0c2b5d0) into develop (2ee3775) will increase coverage by 6.21%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3923      +/-   ##
===========================================
+ Coverage    77.29%   83.50%   +6.21%     
===========================================
  Files          136      136              
  Lines        10446    10446              
  Branches       492      492              
===========================================
+ Hits          8074     8723     +649     
+ Misses        2194     1574     -620     
+ Partials       178      149      -29     

Flag	Coverage Δ
e2e	83.39% <100.00%> (+6.24%)	⬆️
unit	28.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/mermaid/src/diagrams/flowchart/flowDb.js	82.59% <100.00%> (+4.14%)	⬆️

... and 22 files with indirect coverage changes

[mermaid-bot]

@tomperr, Thank you for the contribution!
You are now eligible for a year of Premium account on MermaidChart.
Sign up with your GitHub account to activate.

[brunolnetto]

I see an approval, I like! :-) do the style apply to the arrow as well?