xychart: support for multiple datasets added

[camueller]

📑 Summary

Currently the xychart does not support multiple datasets which is a very common requirement. I also need it myself for a software package which uses mermaid. Therefore I have implemented support for multiple datasets while staying compatible with existing usages.

📏 Design Decisions

There were not really any design changes required. For changes to parser syntax please have a look at demos/xychart.html.

📋 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

[netlify]

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	f1490ff
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/65ba318f63466800088d965f
😎 Deploy Preview	https://deploy-preview-5167--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

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (33d45f6) 43.21% compared to head (f1490ff) 79.52%.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #5167       +/-   ##
============================================
+ Coverage    43.21%   79.52%   +36.31%     
============================================
  Files           23      168      +145     
  Lines         5033    13950     +8917     
  Branches        23      719      +696     
============================================
+ Hits          2175    11094     +8919     
+ Misses        2857     2703      -154     
- Partials         1      153      +152     

Flag	Coverage Δ
e2e	85.32% <93.97%> (?)
unit	43.21% <ø> (ø)

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

Files	Coverage Δ
...s/xychart/chartBuilder/components/plot/PlotType.ts	100.00% <100.00%> (ø)
...ms/xychart/chartBuilder/components/plot/barPlot.ts	100.00% <100.00%> (ø)
...rams/xychart/chartBuilder/components/plot/index.ts	92.85% <100.00%> (ø)
...id/src/diagrams/xychart/chartBuilder/interfaces.ts	100.00% <100.00%> (ø)
...s/xychart/chartBuilder/components/plot/linePlot.ts	89.47% <86.66%> (ø)
packages/mermaid/src/diagrams/xychart/xychartDb.ts	86.25% <78.57%> (ø)

... and 157 files with indirect coverage changes

[sidharthv96]

Love the implementation. Works perfect. But I think the syntax should be modified to improve readability. Actually, there would be no changes to the syntax at all, as the current implementation does support multiple lines. So we only need to tweak the implementation to draw it stacked if there are multiple bar charts present.

Current

     xychart-beta
      title "Basic xychart with multiple datasets"
      x-axis "Relevant categories" [category1, "category 2", category3, category4]
      y-axis Animals 0 --> 160
      bar [["dogs" [40, 20, 40, 30]],["cats" [20, 40, 50, 30]],["birds" [30, 60, 50, 30]]]

Proposed

     xychart-beta
      title "Basic xychart with multiple datasets"
      x-axis "Relevant categories" [category1, "category 2", category3, category4]
      y-axis Animals 0 --> 160
      bar "dogs" [40, 20, 40, 30]
      bar "cats" [20, 40, 50, 30]
      bar "birds" [30, 60, 50, 30]

Then, we could also add a config in future to either draw stacked or grouped charts as necessary.

Found some edge cases with minor bugs

• Zero is not at the bottom, so 0 value is drawn
• When data is missing, bar is drawn at the top.

[camueller]

You are absolutely right about the syntax - somehow I missed your approach to it. It's simple and intuitive from a user perspective and the code is simpler too :-)

Getting the edge cases fixed was a bit tricky. I don't understand why mermaid designed diagrams to not have the Zero at the crossing of the axis. It not just makes the code much more complex but even from a diagram user perspective I don't see any advantage of this approach.

[sidharthv96]

Nice work!

There are two more issues to fix.

1. Multiple line charts don't work now. Repro | Working
2. The bars start at 0 now, but the axis marker is shifted. (I think we should move this and the fix for 0 to a different PR, so it's clearer)

PS: For reviewers having trouble with the test changes, use this option.

[camueller]

Multiple line charts are working again.

You did not answer the question from my previous post:

I don't understand why mermaid designed diagrams to not have the Zero at the crossing of the axis. It not just makes the code much more complex but even from a diagram user perspective I don't see any advantage of this approach.

I would strongly recommend to have the Zero at the crossing of the axis.

In the bar charts I added to the demo I do not notice a shift of axis markers. But as you suggested, if there is something to fix, this can be done in a separate PR.

[sidharthv96]

I don't understand why mermaid designed diagrams to not have the Zero at the crossing of the axis. It not just makes the code much more complex but even from a diagram user perspective I don't see any advantage of this approach.

Sorry, I didn't notice that when it was being developed. Maybe @subhash-halder can answer.

I would strongly recommend to have the Zero at the crossing of the axis.

Me too.

[sidharthv96]

@camueller can you please add some multi line graphs and multi line + stacked bar tests to cypress/integration/rendering/xyChart.spec.js?

@subhash-halder can you please review as well? Thanks!

[subhash-halder]

Hi @camueller Thanks for taking this up.

I have one concern regarding the architecture of the code, how I created the plot is like strategy.
So instead of modifying the components/plot/barPlot.ts it would be better if we could create a different file like components/plot/stackBarPlot.ts and have the logic of drawing stack bar in that file.
so in the future, if we want to add a different variant we could add it easily by not changing the current logic but only adding a new file.

Also, this will have an advantage we can inject the strategy at run time and it will be easy to debug and test as separate file will have separate implementations.

[camueller]

@sidharthv96 I added some more tests. Test code is not compact since [describe|it].each is not supported by your setup.

@subhash-halder I don't think that stackedBarPlot should be another strategy. It also would create redudant code since stackBarPlot would have to include barPlot as well. I wonder why you did not implement support for this right from the start. The initial PR was published by me 4 weeks ago. I will stop contributing at this point and leave it up to you to make this PR fitting to the architecture you have in mind.

[subhash-halder]

@camueller During the initial implementation, we didn't finalize the stack configuration, we simply added support for bar and line charts. My main reason for changing it to a stackBarChart was to consider it as a variation of the bar chart. However, if the team believes that the base implementation of the bar chart should be stacked, then that's okay. I suggested the name stackBarChart to make it clear what the file is actually doing.

I would strongly recommend to have the Zero at the crossing of the axis.

If I understood the point then There could be cases when the effective range does not include zero let's say a chart whose values are [1023, 1043, 1076] then if we want to have zero as a minimum then the visualization would not make much sense, here if the range is from 1000 to 1100 then it's very easy to compare. anyway if you want the range to be from 0 - 1100 it's easy to configure in the chart config. let me know if I am thinking correctly.

And you've done a good job! Please continue with the PR.

[sidharthv96]

There appears to be another edge case as well? Editor

[camueller]

I fixed axis calculation for mix of bar and line chart.

[subhash-halder]

@camueller Please also check with -ve values mixed with +ve values with custom limit and without custom limit
eg

xychart-beta
      title "Basic xychart with multiple datasets"
      x-axis "Relevant categories" [category1, "category 2", category3, category4]
      y-axis -200-->600
      bar "dogs" [40, 20, 40, 130]
      bar "cats" [20, 140, 50, 30]
      bar "birds" [130, 60, 50, 30]

and

xychart-beta
      title "Basic xychart with multiple datasets"
      x-axis "Relevant categories" [category1, "category 2", category3, category4]
      bar "dogs" [40, 20, 40, -130]
      bar "cats" [20, 140, 50, 30]
      bar "birds" [130, 60, 50, 30]

also try with bar and line like

xychart-beta
    title "Sales Revenue"
    x-axis [jan, feb, mar, apr, may, jun, jul, aug, sep, oct, nov, dec]
    bar [5000, 6000, 7500, 8200, 9500, 10500, 11000, 10200, 9200, -8500, 7000, 6000]
    line [5000, 6000, 7500, 8200, 9500, 10500, 11000, 10200, 9200, -8500, 7000, 6000]

once fixed please add some testcases with -ve values also.

[camueller]

@subhash-halder I would suggest you do the remaining work. It was your implementation initially after all and stakced charts should haven been there right from the start.

There are close to non unit tests on your project and even the e2e tests run without expectations (except runtime exceptions). I'm tired of "fix this" after "fix this" and I don't want to be this a neverending story. I'm sure you can fix the remaining bugs in a couple of minutes.

I thought I might contribute something but failed. I have no problem with that.

[subhash-halder]

@subhash-halder I would suggest you do the remaining work. It was your implementation initially after all and stakced charts should haven been there right from the start.

There are close to non unit tests on your project and even the e2e tests run without expectations (except runtime exceptions). I'm tired of "fix this" after "fix this" and I don't want to be this a neverending story. I'm sure you can fix the remaining bugs in a couple of minutes.

I thought I might contribute something but failed. I have no problem with that.

Thank @camueller for taking the time to contribute to this improvement. I genuinely appreciate your willingness to contribute and your honesty in expressing your thoughts.

I understand your concerns regarding the current state of the project, especially regarding the lack of unit tests and the need for stacked charts from the beginning. Your insights are valuable, and I acknowledge the importance of addressing these issues to improve the overall quality of the project.

I want to assure you that your efforts to contribute are greatly appreciated, and there is no failure in attempting to improve the project. Collaboration is key in our development process, and I value your perspective and contributions.

Moving forward, I will work on fixing the remaining bug.

Once again, thank you for your constructive feedback and your willingness to collaborate.

[sidharthv96]

I'm merging this into xychart branch, so that @subhash-halder can work on the fix from there.

[mermaid-bot]

@camueller, Thank you for the contribution!
You are now eligible for a year of Premium account on MermaidChart.
Sign up with your GitHub account to activate.