bug/4849_center_axis_labels

[dreathed]

📑 Summary

This PR centers the axis labels of a quadrant diagram.

Resolves #4849

📏 Design Decisions

There was a condition that would center the labels only if there were no data points.
I removed the condition and replaced the checks for that condition with the code that would execute when the condition would evaluate to true.

Since this issue was marked as a bug I did not add any new test and did not update documentation.

📋 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	2f6c197
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/650d282f58b2b40008875c5a
😎 Deploy Preview	https://deploy-preview-4860--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 #4860 (2f6c197) into develop (4201e47) will decrease coverage by 0.88%.
Report is 10 commits behind head on develop.
The diff coverage is 60.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4860      +/-   ##
===========================================
- Coverage    80.55%   79.67%   -0.88%     
===========================================
  Files          148      148              
  Lines        13024    13025       +1     
  Branches       612      612              
===========================================
- Hits         10491    10378     -113     
- Misses        2400     2510     +110     
- Partials       133      137       +4     

Flag	Coverage Δ
e2e	84.71% <60.00%> (-1.09%)	⬇️
unit	43.67% <ø> (ø)

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

Files Changed	Coverage Δ
...aid/src/diagrams/quadrant-chart/quadrantBuilder.ts	80.53% <60.00%> (-3.40%)	⬇️

... and 12 files with indirect coverage changes

[sidharthv96]

@subhash-halder was there any reason why the option to align left/center was added?
This seems like a deliberate action than a bug.

[subhash-halder]

@sidharthv96 yes it was intentional, we decided to have the label in the center when there are no data points, and they will align to the left when there are data points.

[sidharthv96]

But I can't recollect "why" that decision was made. Was it based on any reference images or standards?

[subhash-halder]

I think it's because we are drawing the labels to the left only and here you suggested if there are no data points it is better to draw the label in center #4383 (comment)

I think if we forced the label to be in a certain place we add a config to do that.

[sidharthv96]

Ok, so the alignment should depend on how many labels are there.

Label Count per axis	Position
1	left/bottom
2	center

If there are 2 labels on the axis, it might not make sense to have points in the graph, with the labels in the current example, but there might exist a usecase where points might make sense.

So, @dreathed, you'll have to revert the changes, and only change the criteria for drawing in center or left.

-    const drawAxisLabelInMiddle = this.data.points.length === 0;
+   const drawAxisLabelInMiddle = <check if label count == 2>;

---

Current behaviour is correct

Fixed behaviour is incorrect (Labels should start from left/bottom)

---

Current behaviour is incorrect (Should be in center, as per issue)

Fixed behaviour is correct

[dreathed]

@sidharthv96 Done.
The condition is now per axis (since may be possible for one axis to have two and the other to have only one label). The condition is: If there is a xAxisRightLabel then the text will be centered. This is because if there is a right label there must be a left label. The same is done for the yAxis.
Just so you know that this condition is not explicitly the number of labels.
If you prefer to bind the condition explicitly to the number of labels I will do 😃 !

[subhash-halder]

@dreathed I think the condition would be to draw the label in the middle only if any of the top or right label is there.

const drawAxisLabelInMiddle = this.data.yAxisTopText || this.data.xAxisRightText

@sidharthv96 can you confirm

[dreathed]

@subhash-halder Well this is about how the output should be if one axis has two and the other has one label. My current solution gives this output:

Your suggestion would give this one:

In the usual case where both axis have either one or two labels, the output will be the same.

[sidharthv96]

I think this solution is good. @dreathed can you add some tests in cypress/integration/rendering/quadrantChart.spec.js to verify the behaviour?

Although the situation where only one axis having 2 labels seem unlikely for a quadrant chart, that's upto the authors of the charts. Our labels should be positioned based on the number of labels on that particular axis.

The other option would be to throw an error if the label count is mismatched, but I don't think we should be mandating that.

[dreathed]

@sidharthv96 I added a couple of new snapshot tests. Of course the old snapshots are effected and need to be replaced. Here are some of the new ones:

[mermaid-bot]

@dreathed, 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.