fix: render the participants in same order as they are created

[ad1992]

📑 Summary

Until v10.2.4, the order of DOM nodes was fine - which means that participant actors were rendered in same order as creation. However in d06bb05 the order was reversed using .lower() which is confusing, hence I am reverting to the previous behaviour.

I am not sure if the change was intentional but I hope this won't break any behaviour and tests are passing

Additionally I have added one more example in docs with actor symbols for sequence diagram as there were none with actor symbols.

Resolves #4946

📏 Design Decisions

Keeping the order same as creation simplifies and makes it more intutive

📋 Tasks

Make sure you

• 📖 have read the contribution guidelines
• 💻 have added necessary unit/e2e tests.
• 📓 have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features.
• 🔖 targeted develop branch

I tried to add a unit test but unfortunately this is dependent on D3.js hence its not working out, I will need to mock the implementation of lowercorrectly to make it work.

[netlify]

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	01bbcc5
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/654ccef2a9124000080d3443
😎 Deploy Preview	https://deploy-preview-5017--mermaid-js.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[codecov]

Codecov Report

Merging #5017 (01bbcc5) into develop (7c7f3dd) will increase coverage by 37.12%.
Report is 60 commits behind head on develop.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #5017       +/-   ##
============================================
+ Coverage    42.94%   80.06%   +37.12%     
============================================
  Files           23      164      +141     
  Lines         4972    13862     +8890     
  Branches        21      698      +677     
============================================
+ Hits          2135    11099     +8964     
+ Misses        2836     2611      -225     
- Partials         1      152      +151     

Flag	Coverage Δ
e2e	86.16% <100.00%> (?)
unit	42.91% <ø> (-0.03%)	⬇️

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

Files	Coverage Δ
.../mermaid/src/diagrams/sequence/sequenceRenderer.ts	88.80% <100.00%> (ø)
packages/mermaid/src/diagrams/sequence/svgDraw.js	88.29% <100.00%> (ø)

... and 158 files with indirect coverage changes

[ad1992]

@sidharthv96 let me know if you have any concerns with this change

[sidharthv96]

d06bb05#diff-1f8b0d46157ff209384aa6d146c2542142552e50ed6257ab525b1b789f5a0cde seems to have multiple places where .lower() is called. @Valentine14th, can you confirm if removing that won't be an issue?

---

@ad1992, a bit more descriptive commit messages than fix would help when debugging later on.

[ad1992]

d06bb05#diff-1f8b0d46157ff209384aa6d146c2542142552e50ed6257ab525b1b789f5a0cde seems to have multiple places where .lower() is called. @Valentine14th, can you confirm if removing that won't be an issue?

@ad1992, a bit more descriptive commit messages than fix would help when debugging later on.

Are you referring to the individual commit messages in PR ? The main commit message communicates what is fixed in this PR (As shown in PR title).
Since those individual commit messages will be squashed so I might have just committed it as fix for minor typos. I will reword it better going forward.

[ad1992]

d06bb05#diff-1f8b0d46157ff209384aa6d146c2542142552e50ed6257ab525b1b789f5a0cde seems to have multiple places where .lower() is called. @Valentine14th, can you confirm if removing that won't be an issue?
@ad1992, a bit more descriptive commit messages than fix would help when debugging later on.

Are you referring to the individual commit messages in PR ? The main commit message communicates what is fixed in this PR (As shown in PR title). Since those individual commit messages will be squashed so I might have just committed it as fix for minor typos. I will reword it better going forward.

Oops sorry just noticed that individual commits are merged as well so all, will make sure the individual commits are descriptive as well.
On other note, you might want to try squashing the commits into a single commit via GH dropdown when merging the PR as it keeps the context of PR clean and links the PR as well with commit message automatically which is very helpful when someone comes back to it later

[sidharthv96]

We are not that strict with the commit messages. Squashing is usually left to the PR author's will without any interference while merging, unless there are lots of small, related commits in the PR. :)

[ad1992]

We are not that strict with the commit messages. Squashing is usually left to the PR author's will without any interference while merging, unless there are lots of small, related commits in the PR. :)

I see got it :)

Also regarding .lower() there is just one more place in drawActorTypeActor and removing that doesn't cause any issue (as per my testing) and all tests pass as well. Let me know If I should push that change or we can keep it for later as well in case we are unsure.

[sidharthv96]

Current

New

That's a problem @ad1992.

[sidharthv96]

The unit tests won't find issues like these sadly. And our visual tests run only before release. So that's why all the tests passed.

[ad1992]

Current New

That's a problem @ad1992.

Hmm interesting, this issue wasn't there in previous versions so probably something changed post that which is breaking it. will check

[ad1992]

I have pushed the fix and rendering the top actor with lines first which is intuitive as those are the first shapes to be drawn.
The order is = Top actor with lines -> messages -> bottom actors

However in the same PR d06bb05 where ordering was changed, the top actors were pushed after messages as well which I have reverted here.

Let me know if this works well. I matched with the diagrams in the mermaid docs and they look good.

[nirname]

LGTM

[ad1992]

Thank you! Any tentative date for the next release?

[aloisklink]

Thank you! Any tentative date for the next release?

We're planning on making the next release a v11/major release, so it might take a bit longer than usual unfortunately. Assuming nothing changes, we're hoping it will be out before the end of the year 🤞

[ad1992]

Thank you! Any tentative date for the next release?

We're planning on making the next release a v11/major release, so it might take a bit longer than usual unfortunately. Assuming nothing changes, we're hoping it will be out before the end of the year 🤞

Thanks for the update @aloisklink! I can see there is a v10 milestone so checking if there is any plan of v10 minor version before v11 ?

[aloisklink]

Thanks for the update @aloisklink! I can see there is a v10 milestone so checking if there is any plan of v10 minor version before v11 ?

It looks like there might be a v10.6.2 patch release and this PR is in it: #5103 🤞 You should be able to use a release candidate pre-release version already by adding "mermaid": "^10.6.2-rc.2" to your package.json file.

[sidharthv96]

@ad1992 there was a regression #5188 (comment). Do you think the fix will be easy?

reproduction: https://deploy-preview-5017--mermaid-js.netlify.app/edit#pako:eNp9kk1rwzAMhv-K6nM6WI4-dLSMsdsG7TEXzVYbU8fKHIculP732SZpS_bhi4X1vNIr4bNQrElI0dFnT07Rs8GDx6ZykM_aGkWwXK1gwx8SXslaTmEBNZ8APcHAPTxNeMxkOMskvBhHBYQa3TFxD7B2OgVXXnnCQNCiD0aZFl2Ad7b8s3t6je1NDhYzOarAHt4sPgJ26Z7yCc7ylEtyY6ZUpu-sxtKBr401dcHzkKmZmeXXWG0z0OKPNaUxHZ_-GzPWKGdub0VSclo2HtC4ua-M_mqszMZgHxfS3PxNuujubgFlFI2OdzUBOS0K0ZBv0Oj4Jc4JrUSoqaFKyBhq9MdKVO4SOewDbwenhAy-p0L0rY4zjt9HyD3aji7fu222Bw

[ad1992]

@ad1992 there was a regression #5188 (comment). Do you think the fix will be easy?

reproduction: https://deploy-preview-5017--mermaid-js.netlify.app/edit#pako:eNp9kk1rwzAMhv-K6nM6WI4-dLSMsdsG7TEXzVYbU8fKHIculP732SZpS_bhi4X1vNIr4bNQrElI0dFnT07Rs8GDx6ZykM_aGkWwXK1gwx8SXslaTmEBNZ8APcHAPTxNeMxkOMskvBhHBYQa3TFxD7B2OgVXXnnCQNCiD0aZFl2Ad7b8s3t6je1NDhYzOarAHt4sPgJ26Z7yCc7ylEtyY6ZUpu-sxtKBr401dcHzkKmZmeXXWG0z0OKPNaUxHZ_-GzPWKGdub0VSclo2HtC4ua-M_mqszMZgHxfS3PxNuujubgFlFI2OdzUBOS0K0ZBv0Oj4Jc4JrUSoqaFKyBhq9MdKVO4SOewDbwenhAy-p0L0rY4zjt9HyD3aji7fu222Bw

@sidharthv96 give me some time, I will look into it and confirm

[ad1992]

So I looked into this and the changes which were made in the d06bb05 does make sense, during actor creation and destroy its very tricky to determine the position of actors, I tried various ways but one or other is failing some case hence i think the author rendered the messages first and updated the position of actors in adjustCreatedDestroyedData and then actors were rendered.

my intention was to make the DOM rendering same as order of creation (which was earlier) but that looks hard and might increase complexities unnecessarily. Hence I would try to go the other way around and change the API instead so we can expose the x/y coords and dimensions as well via the getDiagramFromText API.

const diagram = await mermaid.mermaidAPI.getDiagramFromText(text);
diagram.db.getActors();

Mainly the above getActors function should contain rectData (x, y, width, height, etc) so I can use that directly to compute the dimensions and position, currently rectData is null. @sidharthv96 let me know your thoughts :)

I have a lot of more API ideas to make the API better which I would like to discuss as well.
Also happy to move this to a separate issue if needed.