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: Prevent Legend Labels from Overlapping Diagram Elements in Journey Diagrams #6274

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

Shahir-47
Copy link

@Shahir-47 Shahir-47 commented Feb 13, 2025

📑 Summary

Fixed an issue where long legend labels in Journey diagrams could overlap with the diagram elements. By tracking the maximum width of legend text elements, we now ensure proper spacing between the legend and the diagram content.

Resolves #5955

📏 Design Decisions

The implementation works by:

  1. Tracking the maximum width of legend text elements during the actor legend drawing phase
  2. Using this width to calculate appropriate left margin for the diagram elements
  3. Added e2e tests to verify proper spacing is maintained with both normal and long legend labels

Before:

image

After:

image
image

📋 Tasks

  • [ x ] 📖 have read the contribution guidelines
  • [ x ] 💻 have added necessary unit/e2e tests.
  • [ x ] 📓 have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features.
  • [ x ] 🦋 If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Shahir-47 and others added 4 commits February 5, 2025 19:27
Co-authored-by: Pranav Mishra <[email protected]>
Co-authored-by: Pranav Mishra <[email protected]>
Copy link

changeset-bot bot commented Feb 13, 2025

⚠️ No Changeset found

Latest commit: 5dab86a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Feb 13, 2025
Copy link

netlify bot commented Feb 13, 2025

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 5dab86a
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/67b4f7ca41d9c500081d10cf
😎 Deploy Preview https://deploy-preview-6274--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.

Copy link

pkg-pr-new bot commented Feb 13, 2025

Open in Stackblitz

npm i https://pkg.pr.new/mermaid-js/mermaid@6274
npm i https://pkg.pr.new/mermaid-js/mermaid/@mermaid-js/layout-elk@6274
npm i https://pkg.pr.new/mermaid-js/mermaid/@mermaid-js/mermaid-zenuml@6274
npm i https://pkg.pr.new/mermaid-js/mermaid/@mermaid-js/parser@6274

commit: 5dab86a

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.

Project coverage is 4.46%. Comparing base (52532a7) to head (5dab86a).

Files with missing lines Patch % Lines
...rmaid/src/diagrams/user-journey/journeyRenderer.ts 0.00% 16 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6274   +/-   ##
=======================================
  Coverage     4.46%   4.46%           
=======================================
  Files          385     384    -1     
  Lines        54276   54272    -4     
  Branches       598     623   +25     
=======================================
  Hits          2425    2425           
+ Misses       51851   51847    -4     
Flag Coverage Δ
unit 4.46% <0.00%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
...rmaid/src/diagrams/user-journey/journeyRenderer.ts 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@Shahir-47
Copy link
Author

Hey @sidharthv96,

@pranavm2109 and I have fixed the issue with overlapping legend labels in Journey diagrams #5955. The fix ensures proper spacing by tracking the maximum width of legend text elements during the actor legend drawing phase and using this width to adjust the left margin for the diagram elements. We’ve also added E2E tests to verify that spacing remains correct for both normal and long labels.

All GitHub tests and CICD checks are passing, and this is ready to be merged. Full details are in the PR description—let us know if you have any questions or feedback!

@Shahir-47 Shahir-47 changed the title fix: Prevent legend text overlap in Journey diagrams Prevent legend text overlap in Journey diagrams Feb 13, 2025
@Shahir-47 Shahir-47 changed the title Prevent legend text overlap in Journey diagrams fix: Prevent Legend Labels from Overlapping Diagram Elements in Journey Diagrams Feb 13, 2025
@@ -84,31 +90,32 @@ export const draw = function (text, id, version, diagObj) {
});

drawActorLegend(diagram);
bounds.insert(0, 0, LEFT_MARGIN, Object.keys(actors).length * 50);
leftMargin = conf.leftMargin + maxWidth - 22.328125;
Copy link
Member

Choose a reason for hiding this comment

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

What is 22.328125, please avoid magic numbers.

Comment on lines +106 to +124
cy.then(() => {
renderGraph(
`journey
title Web hook life cycle
section Darkoob
Make preBuilt:5: Darkoob user
register slug : 5: Darkoob userf deliberately increasing the size of this label to check if distance between legend and diagram is maintained
Map slug to a Prebuilt Job:5: Darkoob user
section External Service
set Darkoob slug as hook for an Event : 5 : admin Exjjjnjjjj qwerty
listen to the events : 5 : External Service
call darkoob endpoint : 5 : External Service
section Darkoob
check for inputs : 5 : DarkoobAPI
run the prebuilt job : 5 : DarkoobAPI
`,
{ journey: { useMaxWidth: true } }
);
});
Copy link
Member

Choose a reason for hiding this comment

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

Why is the cy.then required, when it's not used in other functions?

Copy link

argos-ci bot commented Feb 15, 2025

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ⚠️ Changes detected (Review) 2 added Feb 18, 2025, 9:22 PM

Comment on lines +45 to +49
const textElement = svgDraw.drawText(diagram, labelData);
const textLength = textElement.node().getBBox().width;
if (textLength > maxWidth) {
maxWidth = textLength;
}
Copy link
Member

Choose a reason for hiding this comment

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

Not blocker: We should keep this dynamic width part, but should we split the text into more lines, if it's longer that a longer max limit?

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.

not correctly show title
3 participants