Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Changed calculated label gap to be passed from top #16825

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Changed calculated label gap to be passed from top #16825

wants to merge 7 commits into from
+111 −15

Conversation

konrad-amtenbrink
Copy link

@konrad-amtenbrink konrad-amtenbrink commented Apr 1, 2022 •
edited by plainheart

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:

  • bug fixing
  • new feature
  • others

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

  • 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

@echarts-bot
Copy link

echarts-bot bot commented Apr 1, 2022

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.

@konrad-amtenbrink
Copy link
Author

Just FYI: I talked to @FallenMax - this is his work - I just made changes to get the PR approved and resolved merge conflicts.

@konrad-amtenbrink konrad-amtenbrink mentioned this pull request Apr 1, 2022
Closed
5 tasks
@konrad-amtenbrink
Copy link
Author

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!

@DanielBogenrieder
Copy link

Hey,
are there any news on this? We would benefit a lot from this merge.

Ovilia
Ovilia requested changes Jun 20, 2022
View reviewed changes
Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Ovilia mentioned this pull request Jun 29, 2022
Open
@konrad-amtenbrink konrad-amtenbrink dismissed a stale review via 45140f4 July 20, 2022 09:02
@konrad-amtenbrink
Copy link
Author

@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
Copy link
Contributor

Ovilia commented Jul 21, 2022

@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).

@echarts-bot
Copy link

echarts-bot bot commented Jul 21, 2022

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.

@DanielBogenrieder
Copy link

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

@Ovilia Ovilia requested a review from 100pah August 9, 2022 04:12
100pah
100pah requested changes Aug 24, 2022
View reviewed changes
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

142156 is not correct.

src/component/axis/AxisBuilder.ts Outdated
const defaultMargin = 10;
const axis = axisModel.axis;
// const isHorizontal = axis.getRotate() === 0 || axis.getRotate() === 180;
const isHorizontal = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use axis.isHorizontal()

src/component/axis/AxisBuilder.ts Outdated
const name = retrieve(opt.axisName, axisModel.get('name'));

if (!name) {
return;
}

const labelGap = calcDistanceToAxis();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

echarts has no polyfills for those methods yet.
The compatibility of includes seems to be not good enough.
Can be/[oO]utside/.test(nameLocation).

@konrad-amtenbrink konrad-amtenbrink requested review from 100pah and removed request for Ovilia August 25, 2022 08:59
@Ovilia Ovilia added this to the 5.4.1 milestone Sep 1, 2022
Ovilia
Ovilia requested changes Sep 23, 2022
View reviewed changes
Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply. I think the overall logic is almost correct. Only a few details to be improved.

src/component/axis/AxisBuilder.ts
@@ -365,29 +366,47 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
},

axisName(opt, axisModel, group, transformGroup) {
function calcDistanceToAxis() {
const defaultMargin = 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/component/axis/AxisBuilder.ts
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

const nameGap = axisModel.get('nameGap') || 0;
const gap = isOutside ? calcDistanceToAxis(nameGap) : nameGap;

And labelGap is not necessary any more.

src/component/axis/AxisBuilder.ts
const pos = [
nameLocation === 'start'
? extent[0] - gapSignal * gap
: nameLocation === 'outsideStart'
? extent[0] + gapSignal * (gap) + (nameDirection * labelGap)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'start' and 'outsideStart' can use the same logic because labelGap is 0 for 'start'.

src/coord/axisCommonTypes.ts
@@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a new type and export it so that it can be used in endTextLayout.

@Ovilia
Copy link
Contributor

Ovilia commented Sep 23, 2022

Please click "Resolve conversion" in the above reviews if you have corrected them in the new commits.

@plainheart plainheart modified the milestones: 5.4.1, 5.5.0 Nov 8, 2022
@sjcobb sjcobb mentioned this pull request Nov 18, 2022
Open
@bobbyf-ta
Copy link

Any news on this fix?

@DanielBogenrieder
Copy link

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.

@robin-gerling
Copy link

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 #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?

@robin-gerling
Copy link

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

@DanielBogenrieder
Copy link

Hey,

sorry to bring this up again, but are there any news on this? We would really benefit from this being done.

Greetings,
Daniel

@konrad-amtenbrink
Copy link
Author

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

@Ovilia
Copy link
Contributor

Ovilia commented Sep 26, 2023

Sorry for the late reply. There are too many notifications in my inbox so I failed to see this.

@Ovilia
Copy link
Contributor

Ovilia commented Sep 26, 2023 •
edited

@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?

@konrad-amtenbrink
Copy link
Author

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?

@Ovilia Ovilia modified the milestones: 5.5.0, 5.5.1 Jan 18, 2024
@robin-gerling robin-gerling mentioned this pull request Jan 23, 2024
Open
8 tasks
@robin-gerling
Copy link

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.
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 (#9265 (comment), Option (B)).

@DanielBogenrieder
Copy link

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

@DanielBogenrieder DanielBogenrieder mentioned this pull request May 22, 2024
Open
@Ovilia
Copy link
Contributor

Ovilia commented May 27, 2024

Since this PR has not fixed the problem of this concern

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.

I would prefer to continue discussing under #19534 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ovilia Ovilia Ovilia requested changes

@100pah 100pah Awaiting requested review from 100pah

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
PR: first-time contributor PR: revision needed size/M
Projects
None yet
Milestone
5.5.1
Development

Successfully merging this pull request may close these issues.

xAxis.nameGap and yAxis.nameGap should be set automatically given grid.containLabel
7 participants
@konrad-amtenbrink @DanielBogenrieder @Ovilia @bobbyf-ta @robin-gerling @100pah @plainheart