-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Bar fixes #1142
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
Bar fixes #1142
Conversation
* Fixed bug computing the normalisation of bar plots in group mode. * The bug was caused by calling `sieve.put` with bar sizes that didn't take into account the bar base. * Bar plots in stack and relative mode weren't affected by this bug, because traces that set a bar base are excluded from the stack/relative mode.
* Fix hover xLabelVal and yLabelVal so that it takes into account the bar base.
* Updated hover tests in bar_test to test that hover labels honor bar bases.
Thanks @n-riesco ! |
|
||
pointData.y0 = ya.c2p(barPos(di) - barDelta, true); | ||
pointData.y1 = ya.c2p(barPos(di) + barDelta, true); | ||
pointData.yLabelVal = di.p; | ||
} | ||
else { | ||
pointData.y0 = pointData.y1 = ya.c2p(di.y, true); | ||
pointData.yLabelVal = di.s; | ||
pointData.yLabelVal = di.b + di.s; |
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.
@n-riesco this line here is breaking hover for barmode: 'stack'
bar charts.
Can share some insights on why you added this?
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'll have a look and see what's broken. Is di.b
defined? If I remember correctly it isn't for size-0 bars.
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.
Great. Thanks!
Here's the simplest case http://codepen.io/etpinard/pen/Woopae
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.
So the problem is that now the bar size in non-stacked bars is di.b + di.s, whereas in stacked bars is just di.s. We could distinguish both cases just by checking whether the trace defines base. I'll submit a PR.
From n-riesco#4 (comment)