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

Add log axis for radar charts #11324

Merged
merged 3 commits into from Oct 8, 2019
Merged

Add log axis for radar charts #11324

merged 3 commits into from Oct 8, 2019

Conversation

zifix
Copy link
Contributor

@zifix zifix commented Sep 25, 2019

fixes #11298

@@ -40,7 +41,8 @@ function Radar(radarModel, ecModel, api) {

this._indicatorAxes = zrUtil.map(radarModel.getIndicatorModels(), function (indicatorModel, idx) {
var dim = 'indicator_' + idx;
var indicatorAxis = new IndicatorAxis(dim, new IntervalScale());
var indicatorAxis = new IndicatorAxis(dim,
(indicatorModel.axisType === 'log') ? new LogScale() : new IntervalScale());
Copy link
Contributor

@pissang pissang Sep 26, 2019

Choose a reason for hiding this comment

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

Use indicatorModel.get('type') to access the type option in the indicator like this:

indicator: [{ type: 'log' }]

Copy link
Contributor Author

@zifix zifix Sep 26, 2019

Choose a reason for hiding this comment

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

I switched to get as requested.

@pissang
Copy link
Contributor

pissang commented Sep 26, 2019

Thanks.

I've commented on the code.

And please remove the distribution file. It will block us to merge your PR.

@pissang pissang added this to the 4.6.0 milestone Sep 26, 2019
@zifix
Copy link
Contributor Author

zifix commented Sep 26, 2019

Thanks.

I've commented on the code.

And please remove the distribution file. It will block us to merge your PR.

I have removed both files in dist.

@pissang
Copy link
Contributor

pissang commented Sep 27, 2019

Hi @zifix

I meant not to commit these two distribution files. Not delete them.

@zifix
Copy link
Contributor Author

zifix commented Sep 27, 2019

Hi @zifix

I meant not to commit these two distribution files. Not delete them.

I have readded the files.

@pissang
Copy link
Contributor

pissang commented Sep 29, 2019

Great! There is one last thing I think needs to be changed.

The axisType is better to be renamed to type to be same with other axis components like xAxis

@zifix
Copy link
Contributor Author

zifix commented Sep 29, 2019

I was thinking about that when I defined the attribute. I went with axisType because radar is not dedicated to axis attributes (look at e.g. shape). In the future you might like to have multiple radar types and the obvious attribute of choice is taken already.
For perfect consistency, one might need to introduce a radarAxis attribute in some future api-breaking release.

@pissang
Copy link
Contributor

pissang commented Oct 8, 2019

Hi @zifix . Sorry for the late reply. I was on a vacation. I think what you explained is reasonable. Thanks a lot! Going to merge it

pissang
pissang approved these changes Oct 8, 2019
@pissang pissang merged commit d050565 into apache:master Oct 8, 2019
@Ovilia Ovilia modified the milestones: 4.6.0, 4.5.0 Oct 9, 2019
@Ovilia
Copy link
Contributor

Ovilia commented Oct 9, 2019

Moving to v4.5.0 because this PR has been merged before testing v4.5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Radar with Log Axis throws error
3 participants