Skip to content

Feat: multiple value axis alignment #13210

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
Closed
susiwen8 wants to merge 25 commits into from
Closed

Feat: multiple value axis alignment #13210

susiwen8 wants to merge 25 commits into from

Conversation

@susiwen8
Copy link
Contributor

@susiwen8 susiwen8 commented Aug 31, 2020 •
edited
Loading

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Multiple value axis alignment

Fixed issues

Close #11390
Close #10928

Details

calAxisNiceSplitNumber: this function calculates nice split of each value axis. Because user can specify min or max to axis, axis might not be cut evenly. It returns an array of nice split, find out the biggest one, apply to all axis, ideally, each axis has same nice split number (in most case which some option haven't been specified like min, max, minInterval etc ).

resetAxisExtent: this function use calAxisNiceSplitNumber result to set scale again, then get axis final split number(finalSplitNumber)

adjustValueAxes: this function use biggest finalSplitNumber to calculate unionExtent which represent one unit split's length, then base on axis scale extent and axis scale interval, we can get that in this grid each axis proportion. multiply by unionExtent, it's final axis extent

Screen Shot 2020-09-15 at 23 43 28

Screen Shot 2020-09-15 at 23 43 36

Screen Shot 2020-09-15 at 23 43 49

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.

TODO

  • work with min

@echarts-bot
Copy link

echarts-bot bot commented Aug 31, 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.

The pull request is marked to be PR: author is committer because you are a committer of this project.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 12, 2020
@susiwen8 susiwen8 marked this pull request as ready for review September 15, 2020 15:23
pissang
pissang requested changes Sep 16, 2020
View reviewed changes
src/coord/cartesian/Grid.ts
const extent = axis.scale.getExtent();
const interval = (axis.scale as IntervalScale).getInterval();
if (((extent[1] - extent[0]) % interval) === 0) {
extent[1] = extent[1] + (maxSplitNumber - splitNumber[index]) * interval;
Copy link
Contributor

@pissang pissang Sep 16, 2020

Choose a reason for hiding this comment

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

Should be extent[1] += (maxSplitNumber - splitNumber[index]) * interval here?

Also, I'm wondering if there is a case that all the values are negative. And the extent is [-10, 0]. In this case, developers may prefer extent[0] -= (maxSplitNumber - splitNumber[index]) * interval

@pissang
Copy link
Contributor

pissang commented Sep 16, 2020

The result looks good! I've left some of my questions and suggestions.

@susiwen8 susiwen8 requested a review from pissang September 25, 2020 10:05
@susiwen8
Copy link
Contributor Author

susiwen8 commented Sep 25, 2020

Hi @pissang I have solved the problem you mentioned, Could you review this please?

@susiwen8 susiwen8 mentioned this pull request Oct 19, 2020
Closed
@pissang pissang closed this Oct 19, 2020
@pissang pissang reopened this Oct 19, 2020
@pissang
Copy link
Contributor

pissang commented Oct 19, 2020 •
edited
Loading

Seems the last two cases are broken.

image

@susiwen8
Copy link
Contributor Author

susiwen8 commented Oct 19, 2020

Seems the last two cases are broken.

image

@pissang No, those were the line for max
Screen Shot 2020-10-19 at 22 52 27

@susiwen8
Copy link
Contributor Author

susiwen8 commented Oct 19, 2020 •
edited
Loading

Right now, this doesn't work with min. Strange result from dataToCoord

@pissang
Copy link
Contributor

pissang commented Oct 20, 2020 •
edited
Loading

It's strange that the top and bottom ticks of two axes don't align.

I'd suggest adding more cases to define the expected aligning behavior when different axis options min / max / splitNumber / interval / scale / type are set.

BTW: I found it will throw errors in axes.html

@susiwen8
Copy link
Contributor Author

susiwen8 commented Oct 20, 2020

@pissang More test cases added.

@Ovilia Ovilia mentioned this pull request Oct 27, 2020
Closed
@pissang pissang added this to the 5.1.0 milestone Oct 28, 2020
@susiwen8 susiwen8 changed the base branch from next to master November 12, 2020 10:10
@pissang pissang modified the milestones: 5.1.0, 5.x Mar 1, 2021
@maxdor1
Copy link

maxdor1 commented Nov 23, 2021 •
edited
Loading

Any updates on this?

My clients really need this feature because the charts are really ugly without this.

image

And this is the result with another charting library:

image

@pissang pissang modified the milestones: 5.x, 5.3 Nov 24, 2021
@pissang
Copy link
Contributor

pissang commented Nov 24, 2021

@susiwen8 I will take a deep look at this PR and keep the work based on your change these days

@pissang pissang changed the base branch from master to next December 27, 2021 03:17
@pissang pissang mentioned this pull request Dec 29, 2021
Merged
6 tasks
@pissang pissang removed this from the 5.3 milestone Dec 29, 2021
@susiwen8 susiwen8 closed this Jan 1, 2022
@susiwen8 susiwen8 deleted the 11390 branch January 1, 2022 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pissang pissang Awaiting requested review from pissang

Assignees

No one assigned

Labels

PR: author is committer size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Aligning multiple Y axis intervals Axis tick auto align on multiaxis

3 participants

@susiwen8 @pissang @maxdor1