Skip to content

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

Merged
merged 5 commits into from
Nov 14, 2016
Merged

Bar fixes #1142

merged 5 commits into from
Nov 14, 2016

Conversation

n-riesco
Copy link
Contributor

* 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.
@etpinard etpinard added status: reviewable bug something broken labels Nov 14, 2016
@etpinard etpinard added this to the v1.20.0 milestone Nov 14, 2016
@etpinard
Copy link
Contributor

Thanks @n-riesco !

@etpinard etpinard merged commit 0620416 into plotly:master Nov 14, 2016
@n-riesco n-riesco deleted the pr-20161114-bar-fixes branch November 14, 2016 20:24

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;
Copy link
Contributor

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?

Copy link
Contributor Author

@n-riesco n-riesco Nov 17, 2016

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

2 participants