Changed calculated label gap to be passed from top #16825
Conversation
|
Thanks for your contribution! |
|
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, |
There was a problem hiding this comment.
calcDistanceToAxis seems to break the current behavior of position calculation. This should not happen because many developers won't expect this.
I think it might be an easier concept to understand if we provide some new values for nameLocation (e.g., 'outsideEnd', 'outsideCenter', 'outsideStart').
Please also add some test cases for this change.
|
@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 |
|
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: This message is shown because the PR description doesn't contain the document related template. |
konrad-amtenbrink
force-pushed
the
fix-9265-axis-name-gap-updated
branch
from
4ddb516 to
195afde
Compare
konrad-amtenbrink
force-pushed
the
fix-9265-axis-name-gap-updated
branch
from
c9faabd to
3c5406e
Compare
|
Hey @Ovilia, did you already have time to look over the changes? Greetings and Thanks, |
src/component/axis/AxisBuilder.ts
Outdated
| @@ -352,7 +353,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu | |||
| buildAxisMinorTicks(group, transformGroup, axisModel, opt.tickDirection); | |||
|
|
|||
| // This bit fixes the label overlap issue for the time chart. | |||
| // See https://github.com/apache/echarts/issues/14266 for more. | |||
| // See https://github.com/apache/echarts/issues/142156 for more. | |||
src/component/axis/AxisBuilder.ts
Outdated
| const defaultMargin = 10; | ||
| const axis = axisModel.axis; | ||
| // const isHorizontal = axis.getRotate() === 0 || axis.getRotate() === 180; | ||
| const isHorizontal = true; |
src/component/axis/AxisBuilder.ts
Outdated
| const name = retrieve(opt.axisName, axisModel.get('name')); | ||
|
|
||
| if (!name) { | ||
| return; | ||
| } | ||
|
|
||
| const labelGap = calcDistanceToAxis(); |
There was a problem hiding this comment.
Should better call calcDistanceToAxis only when labelGap is required, that is, when nameLocation is 'outsideXxx'. Because calcDistanceToAxis calls estimateLabelUnionRect, which is not a lite operation and may not work right in some case (like in polar)
src/component/axis/AxisBuilder.ts
Outdated
| const rotationDiff = remRadian(textRotate - rotation); | ||
| let textAlign: ZRTextAlign; | ||
| let textVerticalAlign: ZRTextVerticalAlign; | ||
| const inverse = extent[0] > extent[1]; | ||
| const onLeft = (textPosition === 'start' && !inverse) | ||
| || (textPosition !== 'start' && inverse); | ||
| const textIsStart = textPosition.toLocaleLowerCase().includes('start'); |
There was a problem hiding this comment.
echarts has no polyfills for those methods yet.
The compatibility of includes seems to be not good enough.
Can be/[sS]tart/.test(textPosition).
src/component/axis/AxisBuilder.ts
Outdated
| } | ||
|
|
||
| function isNameLocationOutside(nameLocation: string) { | ||
| return nameLocation.toLowerCase().includes('outside'); |
There was a problem hiding this comment.
echarts has no polyfills for those methods yet.
The compatibility of includes seems to be not good enough.
Can be/[oO]utside/.test(nameLocation).
| @@ -365,29 +366,47 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu | |||
| }, | |||
|
|
|||
| axisName(opt, axisModel, group, transformGroup) { | |||
| function calcDistanceToAxis() { | |||
| const defaultMargin = 10; | |||
There was a problem hiding this comment.
We should probably use xAxis.nameGap instead. The document can be updated so that when the nameLocation is outsideXXX, it means the distance between the axis name and the axis labels.
| const nameLocation = axisModel.get('nameLocation'); | ||
| const nameDirection = opt.nameDirection; | ||
| const textStyleModel = axisModel.getModel('nameTextStyle'); | ||
| const gap = axisModel.get('nameGap') || 0; | ||
|
|
||
| const gap = (axisModel.get('nameGap') || 0); |
There was a problem hiding this comment.
const nameGap = axisModel.get('nameGap') || 0;
const gap = isOutside ? calcDistanceToAxis(nameGap) : nameGap;And labelGap is not necessary any more.
| const pos = [ | ||
| nameLocation === 'start' | ||
| ? extent[0] - gapSignal * gap | ||
| : nameLocation === 'outsideStart' | ||
| ? extent[0] + gapSignal * (gap) + (nameDirection * labelGap) |
There was a problem hiding this comment.
'start' and 'outsideStart' can use the same logic because labelGap is 0 for 'start'.
| @@ -35,7 +35,7 @@ export interface AxisBaseOptionCommon extends ComponentOption, | |||
| inverse?: boolean; | |||
| // Axis name displayed. | |||
| name?: string; | |||
| nameLocation?: 'start' | 'middle' | 'end'; | |||
| nameLocation?: 'start' | 'middle' | 'end' | 'outsideStart' | 'outsideMiddle' | 'outsideEnd'; | |||
There was a problem hiding this comment.
Make this a new type and export it so that it can be used in endTextLayout.
|
Please click "Resolve conversion" in the above reviews if you have corrected them in the new commits. |
|
Any news on this fix? |
|
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, |
|
Hey, sorry to bring this up again, but are there any news on this? We would really benefit from this being done. Greetings, |
|
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 (#16825 (comment)) in a new PR #19534, since those changes are very different to those of this PR. |
|
Hey @Ovilia, @100pah, Let me know if we can do anything to speed up the process. Greetings and thanks |
|
Since this PR has not fixed the problem of this concern
I would prefer to continue discussing under #19534 . |
This is just an updated version of this PR: #12236
Everything here is quoted directly from the mentioned PR.
Brief Information
This pull request is in the type of:
What does this PR do?
Is an adapted and working copy of #12236
Fixes #9265 . Axis.nameGap will be now be calculated upon grid.containLabel: true and axis labels.
Fixed issues
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: #9265 (comment)):
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
Related test cases or examples to use the new APIs
N.A.
Others
Merging options
Other information
For further information please look at the original PR of @FallenMax