-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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
Conversation
src/coord/radar/Radar.js
Outdated
@@ -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()); |
There was a problem hiding this comment.
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' }]
There was a problem hiding this comment.
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.
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. |
Hi @zifix I meant not to commit these two distribution files. Not delete them. |
I have readded the files. |
Great! There is one last thing I think needs to be changed. The |
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. |
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 |
Moving to v4.5.0 because this PR has been merged before testing v4.5.0. |
fixes #11298