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

Fix color and arrow for merge commit (gitGraph) #5152

Merged
merged 2 commits into from
Feb 29, 2024

Conversation

macherel
Copy link
Contributor

📑 Summary

Improve the arrow for merge commit of gitGraph

📏 Design Decisions

Simplify the way to choose the arrow color based to be able to be able to fix it for merge commits.
The shape of the arrow is changed for merge commit.

📋 Tasks

Make sure you

Copy link

netlify bot commented Dec 19, 2023

Deploy Preview for mermaid-js ready!

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

@macherel macherel changed the title Fix color and arrow for merge commit Fix color and arrow for merge commit (gitGraph) Dec 19, 2023
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.20%. Comparing base (31a287b) to head (619f097).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5152   +/-   ##
========================================
  Coverage    43.20%   43.20%           
========================================
  Files           23       23           
  Lines         5115     5115           
  Branches        23       23           
========================================
  Hits          2210     2210           
  Misses        2904     2904           
  Partials         1        1           
Flag Coverage Δ
unit 43.20% <ø> (ø)

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

Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

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

Please add some visual tests to verify the change. And a screenshot to the description as well, for reviewers.

packages/mermaid/src/diagrams/git/gitGraphRenderer.js Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/git/gitGraphRenderer.js Outdated Show resolved Hide resolved
@macherel macherel closed this Feb 16, 2024
@macherel macherel force-pushed the git-graph-merge-commit branch from b55cad3 to 6be91bc Compare February 16, 2024 10:53
@macherel
Copy link
Contributor Author

Rebased on the latest develop, removed some codes that is related to another feature (new PR soon), and some visual test added.

@macherel macherel reopened this Feb 16, 2024
@macherel
Copy link
Contributor Author

The tests I added refers to the last two example in the issue I opened (#5154).
2024-02-16
The first one shows a simple merge commit and a merge back commit.
The second shows a merge on a new branch.
Both examples were displayed in the same way, but now it should be much clearer.

Please note that the result of the tests 32 and 33 have changed (the color of the arrow have been fixed)

@macherel macherel requested a review from sidharthv96 February 16, 2024 16:48
@macherel
Copy link
Contributor Author

As expected, 2 tests are failing (I fixed the color of some arrows on a merge commit) but I don't know how to make the tests pass.
Any help is welcome.

@sidharthv96 sidharthv96 merged commit 1857eb1 into mermaid-js:develop Feb 29, 2024
19 of 20 checks passed
Copy link

mermaid-bot bot commented Feb 29, 2024

@macherel, 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.

@sidharthv96
Copy link
Member

The tests will not pass, it's a limitation of how we've built the CI without external dependencies.
I've manually verified the failing cases, looks great!
Nice job. Should be out in 10.9.0 🚀

@rowanfr
Copy link
Contributor

rowanfr commented Feb 29, 2024

@sidharthv96 The tests didn't pass due to randomly generated commit IDs in the git graph resulting in incompatibility between images. The resolution for this is in PR #5344. Can you either revert this commit and modify this PR or merge the other PR and reset the CI cached images for testing? I'm unfortunately unfamiliar with the precise mechanisms to remove previously cached images so I would need to defer to others.

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.

3 participants