-
Notifications
You must be signed in to change notification settings - Fork 867
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(Tooltip): Fixes tooltip when split series charts are used #1324
Conversation
src/plugins/vis_type_vislib/public/vislib/components/tooltip/_pointseries_tooltip_formatter.js
Outdated
Show resolved
Hide resolved
src/plugins/vis_type_vislib/public/vislib/components/tooltip/_pointseries_tooltip_formatter.js
Outdated
Show resolved
Hide resolved
6d3fb99
to
4026e8c
Compare
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.
some comments. Thanks Ashwin.
@@ -55,7 +56,15 @@ export function pointSeriesTooltipFormatter() { | |||
|
|||
if (datum.y !== null && datum.y !== undefined) { | |||
const value = datum.yScale ? datum.yScale * datum.y : datum.y; | |||
addDetail(currentSeries.label, currentSeries.yAxisFormatter(value)); | |||
let label = currentSeries.label; |
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.
seems data.yAxisLabel will give the label directly?
is there a specific reason to use datum?
data:
{xAxisOrderedValues: Array(5), xAxisFormat: {…}, xAxisLabel: 'machine.os.keyword: Descending', yAxisFormat: {…}, yAxisLabel: 'Count', …}
hits: 25
series: (15) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}]
xAxisFormat: {id: 'terms', params: {…}}
xAxisFormatter: val => xConverter.convert(val)
xAxisLabel: "machine.os.keyword: Descending"
xAxisOrderedValues: (5) ['win xp', 'osx', 'win 7', 'win 8', 'ios']
yAxisFormat: {id: 'number'}
yAxisFormatter: val => yConverter.convert(val)
yAxisLabel: "Count"
zAxisFormat: {id: 'terms', params: {…}}
zAxisFormatter: val => zConverter.convert(val)
zAxisLabel: ""
[[Prototype]]: Object
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.
Unfortunately, when we have multiple fields on the y axis, the yAxisLabel
is incorrectly set. The only way we have to correctly get the label then is from the yRaw
value. Thats why i check in the next few lines to check to see if we have more than one y-axis field before using the yRaw
value instead of the default label. I've added the comment below to call this out as well.
src/plugins/vis_type_vislib/public/vislib/components/tooltip/_pointseries_tooltip_formatter.js
Outdated
Show resolved
Hide resolved
a5a6d46
to
7260995
Compare
7260995
to
ba073c8
Compare
src/plugins/vis_type_vislib/public/vislib/components/tooltip/_pointseries_tooltip_formatter.js
Outdated
Show resolved
Hide resolved
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.
Looks good
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.
LGTM! It solves the problem but @AMoo-Miki does raise a good point about loading lodash.
Signed-off-by: Ashwin Pc <ashwinpc@amazon.com>
Signed-off-by: Ashwin Pc <ashwinpc@amazon.com>
ba073c8
to
b7030cb
Compare
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.
LGTM, thanks!
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.
Thanks Ashwin!
@ashwin-pc which version(s) are we targeting? Feel free to add the labels! |
Signed-off-by: Ashwin Pc ashwinpc@amazon.com
Description
On split series charts, the y axis label is incorrect. This change uses the value from the raw table data to correctly assign the value
Issues Resolved
#1262
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr