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

add edgeLabelTextColor that take the color from secondaryTextColor #5057

Conversation

omer-priel
Copy link
Contributor

@omer-priel omer-priel commented Nov 21, 2023

📑 Summary

Fixed the bug in #5052 . see in Design Decisions

Resolves #5052

📏 Design Decisions

For edge labels was only the field edgeLabelBackground in styles for color of the background.
I add new field called edgeLabelTextColor (edgeLabel like edgeLabelBackground and TextColor like textColor, nodeTextColor and secondaryTextColor) that take the value from secondaryTextColor
After that I used this field in the FlowChartStyleOptions and getStyles for add "color" and "fill" to edge labels

This solution fix for flowchart and for flowchart/elk and it can work for the other diagrams that has this bug with edge labels. But, I fixed only flowchart for the simplicity of this PR

I dont know if this bug need new tests?

Note: The tests is passing if you chanage the testTimeout. But thay are fiailing now beacuse of #5053

Sory of my english :)

📋 Tasks

Make sure you

Copy link

netlify bot commented Nov 21, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 989bb89
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65bde7a0ee9038000822cc93
😎 Deploy Preview https://deploy-preview-5057--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.

@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Nov 21, 2023
@omer-priel
Copy link
Contributor Author

I don't know if this bug need new tests?

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes missing coverage. Please review.

Project coverage is 43.18%. Comparing base (3b5cb02) to head (989bb89).
Report is 1796 commits behind head on develop.

Files with missing lines Patch % Lines
packages/mermaid/src/themes/theme-dark.js 0.00% 2 Missing ⚠️
packages/mermaid/src/themes/theme-forest.js 0.00% 2 Missing ⚠️
packages/mermaid/src/themes/theme-neutral.js 0.00% 2 Missing ⚠️
packages/mermaid/src/themes/theme-base.js 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #5057       +/-   ##
============================================
- Coverage    79.54%   43.18%   -36.37%     
============================================
  Files          175       23      -152     
  Lines        14396     5046     -9350     
  Branches       855       23      -832     
============================================
- Hits         11451     2179     -9272     
- Misses        2751     2866      +115     
+ Partials       194        1      -193     
Flag Coverage Δ
e2e ?
unit 43.18% <22.22%> (-0.04%) ⬇️

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

Files with missing lines Coverage Δ
packages/mermaid/src/themes/theme-default.js 94.38% <100.00%> (-1.51%) ⬇️
packages/mermaid/src/themes/theme-base.js 3.80% <0.00%> (-61.87%) ⬇️
packages/mermaid/src/themes/theme-dark.js 2.80% <0.00%> (-67.34%) ⬇️
packages/mermaid/src/themes/theme-forest.js 3.88% <0.00%> (-65.39%) ⬇️
packages/mermaid/src/themes/theme-neutral.js 4.53% <0.00%> (-63.57%) ⬇️

... and 166 files with indirect coverage changes

@nirname nirname requested a review from a team November 21, 2023 16:31
@sidharthv96
Copy link
Member

Hi @omer-priel,
Can you share some example code which works in https://deploy-preview-5057--mermaid-js.netlify.app/ ?

@omer-priel
Copy link
Contributor Author

omer-priel commented Nov 25, 2023

@sidharthv96 An example of code from the issue of the bug:
deploy-preview-5057--mermaid-js.netlify.app example

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.

Not sure why not all the lines were marked as covered by tests. Maybe they are not. PR looks good. Uniform and simple

@nirname nirname self-assigned this Jun 20, 2024
@omer-priel omer-priel closed this Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edge labels in flow chart use primaryTextColor rather than secondaryTextColor
3 participants