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

Fix bar overlap bug when tick fit is false #2306

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rkrabek
Copy link

@rkrabek rkrabek commented Mar 14, 2018

Partial fix for issue #1386

If tick fit is set to false and there are many values, the bars overlap because the bar width is currently calculated based on how many ticks there are instead of the data points.

@codecov-io
Copy link

codecov-io commented Mar 14, 2018

Codecov Report

Merging #2306 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2306      +/-   ##
==========================================
+ Coverage   74.92%   74.95%   +0.03%     
==========================================
  Files          51       51              
  Lines        4227     4233       +6     
==========================================
+ Hits         3167     3173       +6     
  Misses       1060     1060
Impacted Files Coverage Δ
src/shape.bar.js 100% <100%> (ø) ⬆️
src/axis.js 95.77% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2eb2df7...fa83099. Read the comment docs.

@panthony
Copy link
Contributor

@rkrabek Thanks for your contribution. Could you add some to tests to ensure this is actually fixing something?

I tried your fix with the following config:

http://jsfiddle.net/moeumaj7/3/

And in this case it's actually break the display.

@panthony panthony added waiting on submitter R-needs-tests Request for changes: The PR needs tests labels Apr 14, 2018
… tick count, adds tests for bar width when tick fit is false
@rkrabek
Copy link
Author

rkrabek commented May 29, 2018

@panthony Sorry for the delayed response, I've been super busy at work. I've updated the PR to include tests for bar width when tick fit is false.

The config that you provided: http://jsfiddle.net/moeumaj7/3/ fails with my patch for the same reason it fails when you switch tick/fit to true as shown here: http://jsfiddle.net/ox76hL53/
For the sake of not introducing regressions, I've updated the code to take the max of ticks or unique x values so that should address the break in the config you provided.

This patch aims to fix this scenario (with or without the null data points): http://jsfiddle.net/dtttsa9d/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R-needs-tests Request for changes: The PR needs tests waiting on submitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants