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

Implement equally sized bars #4994

Merged
merged 3 commits into from
Dec 2, 2017

Conversation

simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Nov 26, 2017

When barThickness: undefined|null (default), we compute an optimal sample size based on the smallest tick interval reduced to prevent any bar to overlap (bar equally sized). Added support for a special barThickness: 'flex' value (previous default) that globally arranges bars side by side to prevent any gap when percentage options are 1 (variable bar sizes).

Tests: I didn't update failing ones yet since it's not trivial to adjust (computed) expectations with the new implementation. Will do it as soon as we reviewed and validated this PR.

@benmccann @Darkrender @GTDunk @btumbleson @eele94 @weequ @DurgpalSingh: can you guys checkout and test this PR with your specific use cases, and confirm that it works as expected.

/cc @nagix

Fixes #4666
Fixes #4745
Fixes #4758
Fixes #4825
Fixes #4855
Fixes #4907
Fixes #4983
Fixes #4938

image

When `barThickness: undefined|null` (default), we compute an optimal sample size based on the smallest tick interval reduced to prevent any bar to overlap (bar equally sized). Also added support for a special `barThickness: 'flex'` value (previous default) that globally arranges bars side by side to prevent any gap when percentage options are 1 (variable bar sizes).
function computeFitSampleRange(index, ruler, options) {
var thickness = options.barThickness;
var count = ruler.stackCount;
var pixels = ruler.pixels;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably inline pixels and curr since they are only used once. I think that should actually result in smaller minified code too

}

/**
* Computes the "ideal" sample range based on the absolute bar thickness or, if undefined or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help to explain what a sample range is and potentially why there are different fit and flex sample ranges

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relates to the description from the docs, but that's actually not "sample" but "category". It was initially a range, but after some refactor, it's not anymore one: I will rename these 2 methods compute*CategoryTraits (though, I don't like the "category" term here).

@benmccann
Copy link
Contributor

Woohoo! This is awesome!! I think this will be the most exciting part of 2.8. Pretty great to kill 7 birds with 1 stone :-)

@btumbleson
Copy link

btumbleson commented Nov 26, 2017

Pulled and working as expected! Per my thread, I was working with financial data so I've got gaps for the weekend in time-series data. As you can see, there are appropriate spaces in the data, and the remaining bars are all equal sized.

screen shot 2017-11-26 at 2 12 44 pm

...

Edit for an additional test. So I added barThickness: 'flex' and it seems the abormally-thick bars are back. For my use case, I'm very happy leaving this value null, but not sure this is the expected result:

screen shot 2017-11-26 at 2 20 25 pm

@simonbrunel
Copy link
Member Author

@btumbleson Thanks, good to hear that it solves your issue.

barThickness: 'flex': that's the expected result but definitely not what you want. This mode stretches bars to prevent gaps between bars, it looks weird on your screenshot because categoryPercentage and barPercentage are not 1 by default.

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I think I can follow how it works. I haven't tested it yet to see all the cases

var prev, curr, i, ilen;

for (i = 0, ilen = pixels.length; i < ilen; ++i) {
min = i > 0 ? Math.min(min, pixels[i] - pixels[i - 1]) : min;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can start this loop at 1 and then the ternary condition goes away

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, that was originally part of this loop (that's why i > 0), but forgot after moving it into a separate method.

@simonbrunel
Copy link
Member Author

I decided to go for image based unit tests because expectations are now more complicated to calculate and it doesn't seem a good practice to have complex logic in unit tests since this logic can also be bugged.

@simonbrunel
Copy link
Member Author

Looks like Code Climate introduced a new maintainability check: Cognitive Complexity (never seen it before, maybe I missed it). Most of the codebase doesn't pass this check, I'm going to increase its threshold in another PR to align with the Cyclomatic Complexity (10 instead of 5).

@etimberg
Copy link
Member

Good call on the image tests.

@simonbrunel simonbrunel merged commit 15d1056 into chartjs:master Dec 2, 2017
@simonbrunel simonbrunel deleted the bar-equally-sized branch December 2, 2017 11:38
@andig
Copy link
Contributor

andig commented Dec 2, 2017

@simonbrunel wow that's great!

When barThickness: undefined|null (default), we compute an optimal sample size based on the smallest tick interval reduced to prevent any bar to overlap (bar equally sized).

When I have input data, say 1 per day, is it possible to fix bar thickness to day?

@simonbrunel
Copy link
Member Author

It's not possible to explicitly set barThickness to 'day' (that would be a new feature). The bar thickness is computed based on the tick interval, so you will need to set time.unit: 'day' but you will also get one tick per day.

yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
When `barThickness: undefined|null` (default), we compute an optimal sample size based on the smallest tick interval reduced to prevent any bar to overlap (bar equally sized). Also added support for a special `barThickness: 'flex'` value (previous default) that globally arranges bars side by side to prevent any gap when percentage options are 1 (variable bar sizes).
This was referenced Jan 11, 2018
@AlGantori
Copy link

AlGantori commented Jan 22, 2018

Please also consider the issues I am having here as well.
#3747 (comment)
and
#3747 (comment) follow up

Thank you.

@theStorysEnd
Copy link

Are the changes discussed above available publicly yet?
I am having the same issues as the original poster, currently running on 2.7.1.
Thanks.

@andig
Copy link
Contributor

andig commented Feb 10, 2018

You can build the master branch, its wfm

@JannemanDev
Copy link

JannemanDev commented Feb 11, 2018

I build it myself and worked perfectly. Here you can find how:
http://www.chartjs.org/docs/latest/developers/contributing.html

Be sure, if on Windows, to run your Command Prompt as administrator! And install node and npm (npm is bundled with node) first if you don't have it already:
https://nodejs.org/en/
https://www.npmjs.com/get-npm

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
When `barThickness: undefined|null` (default), we compute an optimal sample size based on the smallest tick interval reduced to prevent any bar to overlap (bar equally sized). Also added support for a special `barThickness: 'flex'` value (previous default) that globally arranges bars side by side to prevent any gap when percentage options are 1 (variable bar sizes).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants