-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
6262 - Fixed faces positioning in journey diagram #6263
base: develop
Are you sure you want to change the base?
6262 - Fixed faces positioning in journey diagram #6263
Conversation
|
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6263 +/- ##
==========================================
- Coverage 4.46% 3.88% -0.59%
==========================================
Files 384 399 +15
Lines 54264 42059 -12205
Branches 623 638 +15
==========================================
- Hits 2425 1635 -790
+ Misses 51839 40424 -11415
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Hey @sidharthv96 ! Our team (@nghtlinh @megantriplett @udvale) has been working on issue #6262. We added an alert dialog to let users know that the valid task score range for the journey diagram is 1-5. Also, we set the height for all tasks to 300. Does this match Mermaid's expected behavior? Please let us know if you have any suggestions! ![]() Edited: Update: Instead of a dialog, we now throw an error for invalid inputs (non-integer or out of 0-5 range). This prevents the graph from rendering when constraints aren't met. Could you let us know if this aligns with Mermaid's expected behavior? Thanks!! |
…ug/6262_faces-position-in-journey-diagram
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
…github.com/Shahir-47/mermaid into bug/6262_faces-position-in-journey-diagram
Hi! I'm I would like to apply some automated changes to this pull request, but it looks like I don't have the necessary permissions to do so. To get this pull request into a mergeable state, please do one of the following two things:
|
Co-authored-by: Monica Nguyen <[email protected]> Co-authored-by: Udval Enkhtaivan <[email protected]> Co-authored-by: Megan Triplett <[email protected]>
…ug/6262_faces-position-in-journey-diagram
Co-authored-by: Monica Nguyen <[email protected]> Co-authored-by: Udval Enkhtaivan <[email protected]> Co-authored-by: Megan Triplett <[email protected]>
…github.com/Shahir-47/mermaid into bug/6262_faces-position-in-journey-diagram
…github.com/Shahir-47/mermaid into bug/6262_faces-position-in-journey-diagram
Hi @sidharthv96 ! Our team has completed our solution, but one test—Argos—is still failing. We suspect this is because our solution introduces a new mechanism to handle the error of face icons going out of bounds, which might cause the screenshot Argos test to fail automatically. Would you mind taking a look at our issue? We’d really appreciate it! |
@@ -1,6 +1,6 @@ | |||
import { imgSnapshotTest, renderGraph } from '../../helpers/util.ts'; | |||
|
|||
describe('User journey diagram', () => { | |||
describe('User journey diagram simple tests', () => { |
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.
Please revert this change, this should fix part of argos test, some are shown as added/removed.
cy.wait(500).then(() => { | ||
expect(errorCaught, 'Error should be thrown for a non-integer score').to.equal(true); | ||
}); |
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.
It's not ideal to have time based waits in tests, can you use an event based wait?
Hi @sidharthv96 ! Our team attempted to fix the issue by reverting changes in the simple tests section of journey.spec.js, but the test is still failing. Could you review our fix to see if it aligns with the expected solution? |
📑 Summary
This PR addresses the issue where faces in the Journey Diagram were not constrained below the horizontal activity line. Additionally, it resolves the problem of faces accepting non-integer and negative values.
Resolves #6262
📏 Design Decisions
Before:

After:


📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.