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(flowchart): support linkStyle - to style the previous line defined #4483

Draft
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

Zumbala
Copy link

@Zumbala Zumbala commented Jun 12, 2023

📑 Summary

linkStyle can now be used without a number, use a minus instead to set the style of the last line defined.

Resolves #431

📏 Design Decisions

Changed the jison to alllow minus as a replacement for the number

| LINKSTYLE SPACE MINUS SPACE stylesOpt
      {$$ = $1;yy.updateLink([yy.getEdges().length-1],$5);}

sample:

  ---
  title: Traffic light
  ---
  flowchart TD
      Driving --> TrafficLight{ Orange? }
      TrafficLight --> |No| Go{Green?}
      linkStyle - stroke:blue;
      TrafficLight ---> |Yes| SpeedUp
      linkStyle - stroke:orange;
      Go --> |Yes| JustCruise
      linkStyle - stroke:green;
      Go --> |No| Breaks[Hit the brakes]
      linkStyle - stroke:red;

Graph-66-should-render-a-flowchart-with-linkStyle---no-htmlLabels snap

Make sure you

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

@Zumbala
Copy link
Author

Zumbala commented Jun 12, 2023

@sidharthv96 I did what you asked of me to sync, but I still get an error, which in my opinion is unrelated to my changes.

I am not that well educated in GIT, so maybe I am doing some things incorrect.

Hope you can help me again in solving below issue?

    ⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯
   
    FAIL  packages/mermaid/src/diagram.spec.ts > diagram detection > should throw the right error for incorrect diagram
    Error: Snapshot `diagram detection > should throw the right error for incorrect diagram 1` mismatched


      - Expected  - 1
      + Received  + 1

         `"Parse error on line 2:
         graph TD; A-->
        --------------^
      - Expecting 'AMP', 'ALPHA', 'COLON', 'PIPE', 'TESTSTR', 'DOWN', 'DEFAULT', 'NUM', 'COMMA', 'MINUS', 'BRKT', 'DOT', 
      - 'PUNCTUATION', 'UNICODE_TEXT', 'PLUS', 'EQUALS', 'MULT', 'UNDERSCORE', got 'EOF'"`
      + Expecting 'AMP', 'ALPHA', 'COLON', 'PIPE', 'TESTSTR', 'DOWN', 'DEFAULT', 'MINUS', 'NUM', 'COMMA', 'BRKT', 'DOT', 
     'PUNCTUATION', 'UNICODE_TEXT', 'PLUS', 'EQUALS', 'MULT', 'UNDERSCORE', got 'EOF'"`
     ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

@sidharthv96
Copy link
Member

You're getting this because the test failed. I'm on my mobile now, but it seems like the test is to check an error and the error text has changed after your changes.

You can update the expect in the test to match the new error text and commit.

@sidharthv96 sidharthv96 requested review from knsv and a team June 13, 2023 04:45
* develop:
  Update coveralls
  Ignore bundlephobia
* develop:
  Disable coveralls
@sidharthv96 sidharthv96 mentioned this pull request Jul 2, 2023
@rockmasterflex69
Copy link

What's the status on this? It would be super great to have style be able to be programmatically linked to an edge without having to memorize the indices and hardcode them, and this is certainly a good step in that direction - allowing people to define the style after the edge.

@netlify
Copy link

netlify bot commented Sep 6, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 83682fb
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65b3f5d7756b3000081b1507
😎 Deploy Preview https://deploy-preview-4483--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

sidharthv96 and others added 5 commits September 6, 2023 06:37
* develop: (705 commits)
  chore: Fix unit tests
  chore(deps): update all patch dependencies
  chore: Update docs
  Update docs
  New Mermaid Live Editor for Confluence Cloud (mermaid-js#4814)
  Update link to Discourse theme component (mermaid-js#4811)
  Update flowchart.md (mermaid-js#4810)
  chore: remove unneeded `CommomDB`
  chore: Update docs
  "CSS" instead of "css" in flowchart.md (mermaid-js#4797)
  Update CONTRIBUTING.md
  Update CONTRIBUTING.md
  fix: typos (mermaid-js#4801)
  chore: Align with convention
  fix: Add support for `~test Array~string~`
  chore: Add JSDoc to apply in sequenceDB
  refactor: Tidy up direction handling
  chore: Fix flowchart arrow
  chore: Add test to verify activate
  chore: Update tests snapshot
  ...
@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (91907fe) 43.04% compared to head (83682fb) 80.16%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #4483       +/-   ##
============================================
+ Coverage    43.04%   80.16%   +37.12%     
============================================
  Files           23      167      +144     
  Lines         5018    13885     +8867     
  Branches        21      707      +686     
============================================
+ Hits          2160    11131     +8971     
+ Misses        2857     2600      -257     
- Partials         1      154      +153     
Flag Coverage Δ
e2e 86.17% <100.00%> (?)
unit 43.04% <ø> (ø)

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

Files Coverage Δ
packages/mermaid/src/diagrams/flowchart/flowDb.js 88.44% <100.00%> (ø)

... and 162 files with indirect coverage changes

@nirname
Copy link
Contributor

nirname commented Sep 19, 2023

@rockmasterflex69 we are waiting for a reply from the author of PR. If it is abandoned than at some point one of the team members will probably fix it and. But I'd better not interfere, because there may be different circumstances and different amount of time on hands. So lets wait for the author a little bit. And if nothing happens we will decide what to do next. The original issue is open for everyone, so nothing, even the existence of other PR, prevents you from creating your own. You can use this as an example as well

* develop: (289 commits)
  chore(deps): update all minor dependencies
  chore(deps): update all patch dependencies
  fix: flowchart image without text
  fix Types
  chore: Update pnpm-lock
  chore: Add tests for calculateDeltaAndAngle
  fix: mermaid-js#5064 Handle case when line has only one point
  reset the testTimeout to 5 seconds and change it directly in the test
  update testTimeout from 5 seconds to 10 seconds
  Update all patch dependencies
  fix broken link
  add latest blog post
  Update all minor dependencies
  Update all patch dependencies
  Fix docs
  Update packages/mermaid/src/docs/community/questions-and-suggestions.md
  Update packages/mermaid/src/diagrams/class/classRenderer-v2.ts
  update edge ids
  draw top actors with lines  first followed by messages
  Bump GitHub workflow actions to latest versions
  ...
@sidharthv96
Copy link
Member

@nirname I think this is good to go now. Let's merge?

Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

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

Looks good to me!

By the way @Zumbala, your commits were made with the email: <ha***en@ti**ix.com> (I've hidden some characters, but you can find them by going to https://github.com/mermaid-js/mermaid/commit/125734228e7f163a38eb8b0874c857d052abfd6b.patch or in the git log).

This is completely optional, but if you want to link your contributions to your GitHub account and get a Contributor badge on this repo, you'll need to add your email, see Adding an email address to your GitHub account.

Comment on lines +654 to +657

%%{ init: { 'flowchart': { 'curve': 'stepBefore' } } }%%
graph LR

Copy link
Member

Choose a reason for hiding this comment

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

Hmmmm, I'm not sure what happened here to add these random newlines? Maybe a bad merge or some weird auto-formatting?

Suggested change
%%{ init: { 'flowchart': { 'curve': 'stepBefore' } } }%%
graph LR
%%{ init: { 'flowchart': { 'curve': 'stepBefore' } } }%%
graph LR

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, not a big deal, nevertheless no need for extra lines there

@aloisklink aloisklink changed the title feature/flowchart linkStyle feature(flowchart): support linkStyle - to style the previous line defined Nov 28, 2023
Copy link
Contributor

@nirname nirname left a comment

Choose a reason for hiding this comment

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

We have a breaking change. So we have to decide either to sacrifice one minor case, or fix it

Comment on lines +820 to +822
### TODOs

- [ ] linkStyle interpolate feature docs missing
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that there are plenty of TODO's in the docs. But they are kinda for active maintainers only, or for those guys who have immediate plans on this. Would not it be better to search & raise an issue as well if missing?

Comment on lines +654 to +657

%%{ init: { 'flowchart': { 'curve': 'stepBefore' } } }%%
graph LR

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, not a big deal, nevertheless no need for extra lines there

@@ -169,8 +169,8 @@ that id.
"*" return 'MULT';
"#" return 'BRKT';
"&" return 'AMP';
"-" return 'MINUS';
Copy link
Contributor

@nirname nirname Nov 29, 2023

Choose a reason for hiding this comment

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

Is there any particular reason why MINUS has been moved one line higher apart from keeping all operators together? My assumption is that minus was recognized as another token.

I am afraid this might be a breaking change, since we are changing priority. Previously - was recognized as NODE_STRING, now it is recognized as minus instead.

I don't know how many people were using it and, perhaps, we could sacrifice it, but I've got to mention, that previously this was working, and no it is not (have a look at b node). This works fine in mermaid.live, but not in the branch:

flowchart TD

a[|key:value|Title1]
b[|key:-value|Title2]

we also allow dash as node name (without wrapping in quotes), but that is OK

flowchart TD

- --> 2
- --> 1

Copy link
Member

Choose a reason for hiding this comment

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

@nirname, isn't it possible to edit the grammar to support both usecase?

Copy link
Collaborator

@knsv knsv Feb 29, 2024

Choose a reason for hiding this comment

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

The feature idea is brilliant! Cudos to @Zumbala but this is a blocker.

Any existing diagrams with a minus in the label without quotes will suddenly break. I think there can be many of those. A minus sign is fairly common.
😞

Let's try to modify the grammar to accommodate this!!! The idea is brilliant, and the implementation is good (apart from the blocker). 💯

@nirname
Copy link
Contributor

nirname commented Nov 29, 2023

@sidharthv96 we need to investigate it further. I'am not even sure that the feature that broke had been documented.

* develop: (228 commits)
  Lint
  Remove echo
  RefTest
  Echo event
  Update cypress
  Fix applitools
  docs: fix lint
  docs: move community to Discord
  Swap condition blocks to avoid using negation
  Reposition const declaration to ideal place
  Change repetitive values into consts
  docs: fix swimm link
  Fix Update Browserslist
  Use pnpm/action-setup@v2
  Fix lint
  Cleanup e2e.yml
  Ignore push events on merge queue
  Remove ::
  Remove ::
  Remove ::
  ...
@sidharthv96 sidharthv96 marked this pull request as draft March 25, 2024 10:04
@sidharthv96
Copy link
Member

Converting to draft to avoid accidentally merging.
Blocker : #4483 (comment)

@nirname
Copy link
Contributor

nirname commented Jun 20, 2024

@Zumbala gentle reminder, any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Abandoned
Development

Successfully merging this pull request may close these issues.

Styling specific lines
6 participants