-
-
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
feature/4743_markdown_support_timeline_diagrams #4852
base: develop
Are you sure you want to change the base?
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 #4852 +/- ##
==========================================
- Coverage 5.74% 5.45% -0.29%
==========================================
Files 280 280
Lines 41937 41963 +26
Branches 492 493 +1
==========================================
- Hits 2408 2290 -118
- Misses 39529 39673 +144
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Let's wait till someone else will approve it, always better to verify it twice.
- [Mermaid Charts & Diagrams for Confluence](https://marketplace.atlassian.com/apps/1222572/) | ||
- [Mermaid Charts & Diagrams for Jira](https://marketplace.atlassian.com/apps/1224537/) |
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.
Nothing against it, but this is a bit out of the scope for the task. Let it be here, since you've already added it, but it's better to keep PR directly related to the issue, that would make tracking the issue down much easier
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.
Oh - yeh - I agree. I'm not sure why it is here either? I haven't added it on purpose - has it come through on a merge or automatically somehow? More than happy for them to be removed - I don't know where the links have come from.
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 is probably originates from the merge commit, yep
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.
There was a commit which didn't update the docs. Most probably this was added by the pre-commit hook.
You can ignore these. It will go away once you sync the latest develop.
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.
Can you please update the timeline docs showing that it's possible to do such thing? You can take inspiration from flowchart and mindmap docs.
And can you please verify that other string types of markdown are supported? i.e., italic.
If they're supported, then please add them to cypress/integration/rendering/timeline.spec.ts
textElem.attr('transform', 'translate(' + dx + ', ' + dy + ')'); | ||
} else { | ||
const dx = (node.width - bbox.width) / 2; | ||
const dy = (node.height - bbox.height) / 2; | ||
textElem.attr('transform', 'translate(' + dx + ', ' + dy + ')'); | ||
} |
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.
Duplicated line. It can be set outside the if statement.
textElem.attr('transform', 'translate(' + dx + ', ' + dy + ')'); | |
} else { | |
const dx = (node.width - bbox.width) / 2; | |
const dy = (node.height - bbox.height) / 2; | |
textElem.attr('transform', 'translate(' + dx + ', ' + dy + ')'); | |
} | |
} else { | |
const dx = (node.width - bbox.width) / 2; | |
const dy = (node.height - bbox.height) / 2; | |
} | |
textElem.attr('transform', `translate(${dx}, ${dy})`); |
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.
No, it cannot. Scope of the dx
dy
variables is inside if
statement only. They must be not const
but var
then and be declared before if
statement.
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.
I see, I thought that if we have all possible cases then it would recognize it as an already declared variable. In this case I'm not sure if we should modify it to this:
let dx: number, dy: number;
if (!htmlLabels) {
dx = node.width / 2;
dy = node.padding / 2;
} else {
dx = (node.width - bbox.width) / 2;
dy = (node.height - bbox.height) / 2;
}
textElem.attr('transform', `translate(${dx}, ${dy})`);
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 number of lines is same in both cases, and there are two more variables in the outer scope.
So I don't think the current implementation should definitely be changed.
If making things compact is a goal, we could do something like this.
const dx = (node.width - (htmlLabels ? bbox.width : 0)) / 2;
const dy = (node.height - (htmlLabels ? bbox.height : 0)) / 2;
textElem.attr('transform', `translate(${dx}, ${dy})`);
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.
I'll tidy this up - I'm can't remember why we don't include the bounding box for svgs - it may have just been attempting to make it consistent with before. I'll work this out first and then either simplify or use as per @sidharthv96 suggested code.
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.
And I'll add the documentation / additional tests as per top of conversation.
.attr('alignment-baseline', 'middle') | ||
.attr('dominant-baseline', 'middle') | ||
.attr('text-anchor', 'middle') | ||
.call(wrap, node.width); |
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.
Is there a reason why this line was deleted? Or is it included here:
mermaid/packages/mermaid/src/diagrams/timeline/svgDraw.js
Lines 530 to 534 in 9279166
createText(textElem, description, { | |
useHtmlLabels: htmlLabels, | |
width: node.width, | |
classes: 'timeline-node-label', | |
}); |
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 only required of htmlLabels is false, other the styling is done in css and is in the style.js file. (See the conversation below).
.timeline-node-label { | ||
dy: 1em; | ||
alignment-baseline: middle; | ||
text-anchor: middle; | ||
dominant-baseline: middle; | ||
text-align: center; | ||
} |
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.
Isn't this a duplication of this?
mermaid/packages/mermaid/src/diagrams/timeline/svgDraw.js
Lines 536 to 542 in 9279166
if (!htmlLabels) { | |
textElem | |
.attr('dy', '1em') | |
.attr('alignment-baseline', 'middle') | |
.attr('dominant-baseline', 'middle') | |
.attr('text-anchor', 'middle'); | |
} |
If so, then shouldn't we just keep one of then? Of course add the last two lines.
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.
I'd prefer the stylings to be in the styles file, instead of inside the draw.
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.
One is for if it is using the config setting for htmllabels and is css styling, the other is for if it is rendered as an svg (I'm not sure if this can be targeted with css - I'll check). To be honest I'm not sure why there is a config setting for htmllabels, I'm assuming a historical reason - createText needs it and I copied the pattern from mindmaps. I could just always assume it will be rendered in html? (I.e. remove the whole of the htmllabels config for timelines and just always pass a true into createText).
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.
I wouldn't recommend testing this feature like this, you're kinda testing createText
right now.
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.
This is a good point - the actual thing I am putting under test is svgDraw.drawNode as this was the smallest self contained unit that was easy to test. If this not the appropriate domain level thing to test behaviour at, there are a couple of options.
The rest of the (previously existing) tests in timeline.spec.js are testing the parser. The tests won't fit here as they are to do with rendering not semantics.
I could test a level higher and test draw in timelineRenderer.ts (although this becomes tricky as a self enclosed unit because it goes and fetches config). Or...
The other option is to not unit test and test the behaviour only with e2e tests. If you worried that I am too close to testing implementation this might be appropriate?
* develop: (434 commits) Changes to gantt.html 1. Added a Gantt diagram that demonstrates to users that hashtages and semicolons can be added to titles, sections, and task data. Changes to gantt.spec.js 1. Added unit tests to ensure that semicolons and hashtags didn't break the functionality of the gantt diagram when used in titles, sections or task data. Changes to /parser/gantt.spec.js 1. Added rendering tests to ensure that semicolons and hashtags in titles, sections, and task data didn't break the rendering of Gantt diagrams. perf: prevent adding multiple DOMPurify hooks Lint Remove echo RefTest Echo event Update cypress Fix applitools docs: fix lint docs: move community to Discord Swap condition blocks to avoid using negation Reposition const declaration to ideal place Change repetitive values into consts docs: fix swimm link add sequenceDiagram link e2e test Fix Update Browserslist Use pnpm/action-setup@v2 Fix lint Cleanup e2e.yml Ignore push events on merge queue ...
@jonnymoo I'd like to revive this PR, could you update your branch to the latest development branch? |
Hey Nikolay - yes will do. Apologies, I've been onto the project for a little while, so I'll just need to get my head back into it. I'll try and get this done in the next week.
Cheers,
Jonny
…________________________________
From: Nikolay Rozhkov ***@***.***>
Sent: 20 June 2024 15:20
To: mermaid-js/mermaid ***@***.***>
Cc: Jonny Muir ***@***.***>; Mention ***@***.***>
Subject: Re: [mermaid-js/mermaid] feature/4743_markdown_support_timeline_diagrams (PR #4852)
@jonnymoo<https://github.com/jonnymoo> I'd like to revive this PR, could you update your branch to the latest development branch?
—
Reply to this email directly, view it on GitHub<#4852 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AASGWJM4Q3GJJHZVMQX6RH3ZILQKNAVCNFSM6AAAAAA4446MT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBQHAZTKNJQHA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Conciseness: The ternary operator condenses the logic for calculating dx and dy into a single line, making the code more compact and easier to read. Readability: The use of template literals () allows for cleaner string formatting within the transform attribute. This makes it easier to see how the variables are being used. Maintainability: Since the logic for both conditions is in one place, it simplifies future modifications if the calculation needs to be changed.
Conciseness: The ternary operator condenses the logic for calculating dx and dy into a single line, making the code more compact and easier to read. Readability: The use of template literals () allows for cleaner string formatting within the transform attribute. This makes it easier to see how the variables are being used. Maintainability: Since the logic for both conditions is in one place, it simplifies future modifications if the calculation needs to be changed.
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
I have rebased and cleaned up some merge conflicts. Hope this is correct. |
I would find utility in this work being picked back up, and also for the ability to have hyperlinks within SVG digrams when doing so. I've opened #6343 and #6352 to support this. For anyone wanting this functionality in the meantime, I've also released these features under by own namespace:
|
📑 Summary
This pull request makes timeline diagrams behave the same as mind maps and flow diagrams in terms of adhering to markdown content.
For example:
Should render as:
Resolves #4743
📏 Design Decisions
This implementation changes the drawNode function in timelines to follow the same pattern as mind maps.
Instead of doing its own text drawing it now re-uses CreateText in rendering-util/createText.js which allows some markup rendering.
Additionally a configuration for timeline/htmlLabels, default true has been introduced to make it work exactly like flow diagrams.
This only allows a subset of markdown to be used as per existing rendering-util/handle-markdown-texts.ts code. This makes it the safest to do (there is no chance of xss for instance) and if more is required in the future then it can be added in the utils in a safe manner.
Note - to make unit testing simpler for testing the rendering, I have removed the mock for the d3 library and addressed the flow diagram test that was reliant on it. This can be re-added on a case by case basis if required, but the patterns I have used might be quite useful.
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.develop
branch