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 12109 #12418

Merged
merged 20 commits into from
May 28, 2020
Merged

Fix 12109 #12418

merged 20 commits into from
May 28, 2020

Conversation

gracelia
Copy link
Contributor

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

correct bar polar rendering negative values

Fixed issues

#12109

Details

Before: What was the problem?

image

After: How is it fixed in this PR?

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 Apr 12, 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.

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.

Thanks for your contribution. The basic idea is all right but some modification is required.

src/layout/barPolar.js Outdated Show resolved Hide resolved
src/layout/barPolar.js Outdated Show resolved Hide resolved
src/layout/barPolar.js Outdated Show resolved Hide resolved
test/polarLine2.html Outdated Show resolved Hide resolved
test/polarLine2.html Outdated Show resolved Hide resolved
@pissang pissang added this to the 4.9.0 milestone Apr 15, 2020
@gracelia
Copy link
Contributor Author

  1. remove min max calculation logic with axis.scale.getExtent()
  2. moved cases to bar-polar-stack.html
  3. add more cases

@susiwen8
Copy link
Contributor

Did you set correct user.name and user.email?

@gracelia
Copy link
Contributor Author

Did you set correct user.name and user.email?

Sorry, not quite sure of the problem.

@susiwen8
Copy link
Contributor

I mean the user.name and user.email of you git. It seems like you didn't use your github account to push commit

@gracelia
Copy link
Contributor Author

I mean the user.name and user.email of you git. It seems like you didn't use your github account to push commit

I got your point. Should I change account and commit again?

@susiwen8
Copy link
Contributor

I mean the user.name and user.email of you git. It seems like you didn't use your github account to push commit

I got your point. Should I change account and commit again?

That won't be necessary, Squash all commits then merge can do the job. I think.

@gracelia
Copy link
Contributor Author

I mean the user.name and user.email of you git. It seems like you didn't use your github account to push commit

I got your point. Should I change account and commit again?

That won't be necessary, Squash all commits then merge can do the job. I think.

Thanks, I'll make a try.

@susiwen8
Copy link
Contributor

I mean the user.name and user.email of you git. It seems like you didn't use your github account to push commit

I got your point. Should I change account and commit again?

That won't be necessary, Squash all commits then merge can do the job. I think.

Thanks, I'll make a try.

Or you can force push.

fix: 12109, add test case

Revert "fix: 12109 add test case"

This reverts commit 66f5c04.

fix: 12109, add case in issue
@gracelia
Copy link
Contributor Author

gracelia commented Apr 21, 2020

I mean the user.name and user.email of you git. It seems like you didn't use your github account to push commit

I got your point. Should I change account and commit again?

That won't be necessary, Squash all commits then merge can do the job. I think.

Thanks, I'll make a try.

Or you can force push.

Do you know where to see if I had done right?

@susiwen8
Copy link
Contributor

Screen Shot 2020-04-21 at 16 21 20
Screen Shot 2020-04-21 at 16 21 29

@gracelia
Copy link
Contributor Author

@susiwen8 Clear, thanks.

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.

Some test cases don't have expected result.

Please compare with bar under grid: (change the data and min max)

option = {
    xAxis: {
        data: ['A', 'B', 'C']
    },
    yAxis: {
        min: 1,
        max: 6
    },
    series: [{
        name: '模拟数据',
        type: 'bar',
        stack: 'a',
        data: [2, 2, 2]
    }, {
        stack: 'a',
        type: 'bar',
        data: [2, 2, 2]
    }, {
        stack: 'a',
        type: 'bar',
        data: [2, 2, 2]
    }]
};

src/layout/barPolar.js Outdated Show resolved Hide resolved
src/chart/bar/BarView.js Outdated Show resolved Hide resolved
src/layout/barPolar.js Show resolved Hide resolved
test/bar-polar-stack.html Show resolved Hide resolved
src/chart/bar/BarView.js Outdated Show resolved Hide resolved
pissang
pissang previously approved these changes May 28, 2020
@Ovilia Ovilia merged commit 184d935 into apache:master May 28, 2020
@echarts-bot
Copy link

echarts-bot bot commented May 28, 2020

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

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.

4 participants