Skip to content
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

Bug in calculateBarIndexPixels #4825

Closed
benmccann opened this issue Oct 7, 2017 · 6 comments
Closed

Bug in calculateBarIndexPixels #4825

benmccann opened this issue Oct 7, 2017 · 6 comments

Comments

@benmccann
Copy link
Contributor

benmccann commented Oct 7, 2017

Context

@tchan originally reported this bug in #4814. There are two separate issues in his example, so I'm opening this bug to track the bar chart issue

Steps to Reproduce (for bugs)

Here's a fiddle: https://codepen.io/anon/pen/JrpVza

Expected Behavior

The data point for 2017-01-17 18:00:00 should be charted before the tick for 2017-01-18

Current Behavior

The data point for 2017-01-17 18:00:00 is charted next to the tick for 2017-01-19

Possible Solution

I believe the bug is in calculateBarIndexPixels. The 17th element (i.e. the one with index=16) has a fullBarSize of 265 pixels, which is why it is mis-positioned by hundreds of pixels.

It's very strange that the bars in this chart have different sizes. I don't understand the intended behavior here, but I don't think it's a good default. If you do a Google Image search for 'bar chart' then all the examples have all the bars being the same size. Different widths is extremely uncommon

screenshot from 2017-10-07 12-39-17

@benmccann
Copy link
Contributor Author

@nagix @simonbrunel any ideas? I believe you guys had some additional plans for the bar charts that never got finished. @nagix would you be able to send the remaining PRs for the next steps of your plan?

@etimberg
Copy link
Member

etimberg commented Oct 8, 2017

I did some investigation. If you remove maxBarThickness then the center is correct for the size of the bar. However, it seems that calculateBarIndexPixels doesn't account for bar thickness being overridden by the settings when calculating the position.

@etimberg
Copy link
Member

etimberg commented Oct 8, 2017

replacing the end of calculateBarIIndexPixels with

if (size === fullBarSize) {
  base -= leftCategorySize;
  base += fullBarSize * stackIndex;
  base += (fullBarSize - size) / 2;
} else {
  base += size * (stackIndex - 0.5);
}

improves things, however there is a chance that bars overlap. Also, the bars at the end are only half visible

chart

@benmccann
Copy link
Contributor Author

That looks way better!

@simonbrunel simonbrunel removed this from the Version 2.7.1 milestone Oct 9, 2017
@simonbrunel
Copy link
Member

Looks like a bug, the bar "anchor" point should be the associated tick value, so when specifying an explicit bar thickness, the bar should be centered around that point. I don't think @etimberg solution really solves the issue but instead creates more problems, like you said it will generate overlaps when bars have different sizes. Maybe @nagix have a better idea why the bar is not centered around the expected time when a bar thickness is specified?

image

@simonbrunel simonbrunel added this to the Version 2.7.1 milestone Oct 9, 2017
@simonbrunel
Copy link
Member

We should target to fix that bug for 2.7.1, not change the default behavior.

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

No branches or pull requests

3 participants