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.

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

fix #9265 axis name overlapped with axis labels for grid.containLabel: true #12236

Closed
wants to merge 1 commit into from

Conversation

FallenMax
Copy link

@FallenMax FallenMax commented Mar 4, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

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)):

image

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.

image

Usage

Are there any API changes?

  • The API has been changed.

Related test cases or examples to use the new APIs

NA.

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

@echarts-bot
Copy link

echarts-bot bot commented Mar 4, 2020

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.

@FallenMax FallenMax changed the title fix #9265 axis.nameGap set automatically given grid.containLabel fix #9265 axis name overlapped with axis labels for grid.containLabel: true Mar 4, 2020
@pissang pissang added this to the 4.9.0 milestone Mar 10, 2020
function calcDistanceToAxis() {
var axis = axisModel.axis;
if (axis.grid.model.get('containLabel') && !axis.model.get('axisLabel.inside')) {
var labelUnionRect = estimateLabelUnionRect(axis);
Copy link
Contributor

@pissang pissang Aug 4, 2020

Choose a reason for hiding this comment

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

AxisBuilder can also be used on coordinate systems other than grids. In which case, axis.grid may be null. And access grid model in the axis builder is an abstraction leak.

It's better to pass the extra gap calculated from labels from the top. Which can be a parameter in https://github.com/apache/incubator-echarts/blob/master/src/component/axis/CartesianAxisView.js#L60

Copy link
Author

Choose a reason for hiding this comment

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

Sorry but I don't have time looking into this for now, can you make a fix if the issue is still relevant?

@pissang pissang modified the milestones: 4.9.0, TBD Aug 5, 2020
@adyry
Copy link

adyry commented Sep 25, 2020

I was really waiting for this fix to come with 4.9.0...

@mpolverini
Copy link

This is still a relevant problem. Is there any chance that this fix is merged in the short term?

@konrad-amtenbrink
Copy link

Hey, as this is still a relevant problem - I want to implement the requested changes for this PR. Is there a way this is possible? @pissang @FallenMax

@FallenMax
Copy link
Author

@konrad-amtenbrink No problem! If it's needed, I'v added you as collaborator to https://github.com/FallenMax/incubator-echarts, feel free to use this branch to make/push any change, or fork echarts and make a separate PR. (Sorry for not being able to finishing the PR myself)

@konrad-amtenbrink
Copy link

konrad-amtenbrink commented Apr 1, 2022

@FallenMax @pissang After looking at the PR I forked the repository myself and applied the changes already made. There is a new PR open for this issue. I hope this works out for everybody, if not please let me know. The new PR is: #16825
So this PR is no longer needed.

@plainheart plainheart closed this Apr 2, 2022
@Ovilia Ovilia removed this from the TBD milestone Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xAxis.nameGap and yAxis.nameGap should be set automatically given grid.containLabel
7 participants