-
Notifications
You must be signed in to change notification settings - Fork 13
Upgrades Radar Vis to Kibana 6.2.2 #43
base: master
Are you sure you want to change the base?
Upgrades Radar Vis to Kibana 6.2.2 #43
Conversation
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.
Hi @varundbest
Thank you for your contribution
package.json
Outdated
@@ -14,28 +14,5 @@ | |||
"repository": { | |||
"type": "git", | |||
"url": "[email protected]:sirensolutions/kibi-radar-chart-plugin.git" | |||
}, | |||
"scripts": { |
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.
Please leave the script and devDependencies section
They are used by siren team
public/kibi_radar_vis.js
Outdated
name: 'radar', | ||
title: 'Kibi Radar Chart', | ||
description: 'A radar chart is a graphical method of displaying multivariate data ' + | ||
'in the form of a two-dimensional chart of three or more ' + | ||
'quantitative variables represented on axes starting from the same point.' + | ||
' The relative position and angle of the axes is typically uninformative.', | ||
icon: 'fa-empire', | ||
category: VisType.CATEGORY.KIBI, | ||
category: CATEGORY.KIBI, |
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 CATEGORY.SIREN
public/kibi_radar_vis.js
Outdated
addLevelScale: 1, | ||
addLabelScale: 0.9, | ||
addLevelNumber: 5 | ||
radar: { |
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.
Why you introduced this wrapping object? Is this required by kibana6?
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.
Yes, apparently kibana 6 starts rendering its own legend after detecting these params.
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.
Can the name of the property be anything else instead of radar?
I mean does it have to match the name property?
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.
No, I just named it 'radar' as it's a radar vis. What name would you prefer?
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.
Radar is fine. I just wanted to know if it has to match the name
Actually, if you could just do the test to double check would be nice.
Like renaming it temporarily to something else like "radar1" and check that it would still work
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.
Sure, changed the variable to 'config' as it is somewhat descriptive.
It has come to my notice that you're not exactly using these configs:
fontSize
shareYAxis
addTooltip
isDonut
So i have removed them.
…igs, restored package.json, changed CATEGORY to SIREN.
@varundbest Nice Thanks all good now |
Perhaps you can park it at a release/6.2.2 branch till then. |
jenkins test it |
Upgrades Radar Vis to Kibana 6.2.2