apache
Changed calculated label gap to be passed from top
This is just an updated version of this PR: https://github.com/apache/echarts/pull/12236 Everything here is quoted directly from the mentioned PR.
Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
What does this PR do?
Is an adapted and working copy of https://github.com/apache/echarts/pull/12236 Fixes https://github.com/apache/echarts/issues/9265 . Axis.nameGap will be now be calculated upon grid.containLabel: true and axis labels.
Fixed issues
- https://github.com/apache/echarts/issues/9265 xAxis.nameGap and yAxis.nameGap should be set automatically given grid.containLabel
Details
Before: What was the problem?
For charts with grid.containLabel set to true, axis name could be overlapped with axis labels, if yAxis.nameGap is not manually tweaked.
Here is how echarts behaves by default (quoting: https://github.com/apache/echarts/issues/9265#issuecomment-433346995):
After: How is it fixed in this PR?
now axis' name will always placed outside grid + axis label rect. nameGap only adds some additional gap.
Misc
- [ ] The API has been changed (apache/echarts-doc#xxx).
- [ ] This PR depends on ZRender changes (ecomfe/zrender#xxx).
Related test cases or examples to use the new APIs
N.A.
Others
Merging options
- [ ] Please squash the commits into a single one when merging.
Other information
For further information please look at the original PR of @FallenMax
Thanks for your contribution! The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.
![echarts-bot[bot] avatar](https://avatars.githubusercontent.com/in/20872?v=4 "Published by a pub.dev verified publisher"){kind=small}
Just FYI: I talked to @FallenMax - this is his work - I just made changes to get the PR approved and resolved merge conflicts.
Hey, I was just wondering if you (@pissang) could have a quick look at this PR - since you requested the changes in the first place - thank you very much!
Hey, are there any news on this? We would benefit a lot from this merge.
@Ovilia I added your requested changes - could you please specify where you want me to add a test? I could not find tests for the axis labels or anything related
@Ovilia I added your requested changes - could you please specify where you want me to add a test? I could not find tests for the axis labels or anything related
Hi. Thanks for your update! Please check if axisLabel or axis-containLabel is related. If neither is, you can create a new one by npm run mktest axis-label-gap. Please make sure to keep the option minimal (with the least required data and other options).
To reviewers: If this PR is going to be described in the changelog in the future release, please make sure this PR has one of the following labels: PR: doc ready, PR: awaiting doc, PR: doc unchanged
This message is shown because the PR description doesn't contain the document related template.
Hey @Ovilia,
did you already have time to look over the changes? We would really benefit a lot from this merge.
Greetings and Thanks, Daniel
Please click "Resolve conversion" in the above reviews if you have corrected them in the new commits.
Sorry this is on us, we didn't have time yet to implement the changes requested. We will hopefully find enough time to fix them this or next week.
Hey @Ovilia, hey @100pah, I will continue resolving the comments of this PR, but I have a few questions. I have read through the comments of PR https://github.com/apache/echarts/pull/12236 and this PR and wonder why outsideStart and outsideEnd were added. Was this requested by you? To me, outsideStart doesn't make sense since the x-axis name is left of the y-axis and the y-axis name is below the x-axis (when using start) but I cannot see how to access the LabelUnionRect of the other axis (the axes are also created sequentially, which is why we cannot access the second axis from the first during the creation of the first axis). Do I miss something regarding this problem? For outsideEnd, the logic gets quite complex since there should be a differentiation between positive/negative rotation of the labels, category vs value axis-type (since e.g. the category labels are centred between axis ticks while labels for the value type are centred below the ticks). Also in case the y-axis is not at the left but on the right, the same problem as for outsideStart arises. Based on those findings I would remove outsideStart and outsideEnd from the PR. Do you agree with removing it?
Hey @Ovilia, hey @100pah,
Had you already time to look at the problems mentioned in my previous post? What do you think about removing outsideStart and outsideEnd?
Greetings, Robin
Hey,
sorry to bring this up again, but are there any news on this? We would really benefit from this being done.
Greetings, Daniel
Hey @Ovilia and @100pah , are there any updates on this? Anything we can do to resolve the problems to get the PR merged? Just let me know - thanks!
Konrad
Sorry for the late reply. There are too many notifications in my inbox so I failed to see this.
@konrad-amtenbrink I got a little confused here. It seems to me that we should do something to the grid area so that it shrinks to make space for the axis names. But instead, this pull requests suggest centering aligns for the axis names without changing the grid area. So I would like to confirm if this is intended, and what if the axis name is too long to be displayed within the canvas?
Hey @Ovilia , the implementation is intended like this. Also, this is not my original implementation, I just updated the PR from 2.5 years ago openend by FallenMax. Is there any chance that we will merge this or otherwise fix this issue?
Hey @Ovilia, I implemented the changes as requested by you (https://github.com/apache/echarts/pull/16825#issuecomment-1735033805) in a new PR https://github.com/apache/echarts/pull/19534, since those changes are very different to those of this PR. In the new PR, the grid area shrinks according to the space needed by the labels and the name which was also suggested by @100pah (https://github.com/apache/echarts/issues/9265#issuecomment-434022070, Option (B)).
Hey @Ovilia, @100pah, I assume you are both busy, but it would really be nice for us to have this feature implemented at some point. We have revised the PR and hope that it now fits the purpose. Would be great if you could have another look and we can see some progress here.
Let me know if we can do anything to speed up the process.
Greetings and thanks