-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
feat: add name attribute and class "actor-line" to line #5338
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5338 +/- ##
========================================
Coverage 44.67% 44.67%
========================================
Files 25 25
Lines 5339 5341 +2
Branches 27 27
========================================
+ Hits 2385 2386 +1
- Misses 2953 2954 +1
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@sidharthv96 how can I update the failing snapshots ? |
@ad1992 you can't fix snapshots in CI, as they are regenerated from the base branch. But we'll review and merge it, no issues. @mermaid-js/atlantians this an example of the change. All the failing tests contain this exact change in line color. |
Awesome, thanks @sidharthv96 , let me know if there is anything needed from my side to merge this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look good to me! Thanks for catching this.
The visual changes are okay to me, but I'd very slightly prefer if we keep existing sequence diagrams looking the same (it would also mean the E2E snapshots won't change!), by changing the default actorLineColor
var to match what it was for the last few years (e.g. the same colour as the actorBorder
). But I'm happy to be convinced otherwise.
@sidharthv96, @ad1992, any thoughts on what looks nicer?
I have made the changes, however I am getting "Resource not accessible by integration" due to which E2E is failing, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ad1992, don't worry, I don't think it's your fault! It looks like #5235 added a new feature to the E2E CI tests that doesn't work for PRs from forks.
I've made a PR that should fix this in #5365.
All of your changes look great to me, but I'll have to wait until #5365 is merged before I can merge this.
Alright lets merge this @aloisklink @sidharthv96 😊 |
📑 Summary
actor-line
class was never added to the line as it was getting overridden so now that's fixed as wellcc @sidharthv96
📏 Design Decisions
similar to #5284, this is adding a unique identifier for line nodes connecting the actors
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.develop
branch