Implement equally sized bars #4994

simonbrunel · 2017-11-26T15:08:15Z

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

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).

benmccann · 2017-11-26T15:35:45Z

src/controllers/controller.bar.js

+function computeFitSampleRange(index, ruler, options) {
+	var thickness = options.barThickness;
+	var count = ruler.stackCount;
+	var pixels = ruler.pixels;


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

benmccann · 2017-11-26T15:37:38Z

src/controllers/controller.bar.js

+}
+
+/**
+ * Computes the "ideal" sample range based on the absolute bar thickness or, if undefined or


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

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 · 2017-11-26T15:40:00Z

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 · 2017-11-26T19:15:41Z

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.

...

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:

simonbrunel · 2017-11-26T19:47:34Z

@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.

etimberg

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

etimberg · 2017-11-26T22:39:16Z

src/controllers/controller.bar.js

+	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;


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

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

simonbrunel · 2017-11-30T20:20:39Z

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 · 2017-11-30T20:47:53Z

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 · 2017-11-30T23:02:13Z

Good call on the image tests.

andig · 2017-12-02T12:13:59Z

@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 · 2017-12-02T13:04:27Z

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.

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).

AlGantori · 2018-01-22T08:34:05Z

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

Thank you.

theStorysEnd · 2018-02-10T21:00:15Z

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 · 2018-02-10T21:16:27Z

You can build the master branch, its wfm

JannemanDev · 2018-02-11T10:57:34Z

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

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).

simonbrunel added type: bug type: enhancement labels Nov 26, 2017

simonbrunel added this to the Version 2.8 milestone Nov 26, 2017

simonbrunel requested review from benmccann and etimberg November 26, 2017 15:08

simonbrunel force-pushed the bar-equally-sized branch from 7e0ca21 to 98227b1 Compare November 26, 2017 15:23

benmccann reviewed Nov 26, 2017

View reviewed changes

etimberg reviewed Nov 26, 2017

View reviewed changes

simonbrunel added 2 commits November 27, 2017 09:57

Code review: minification, loop and naming

867a369

Add image based unit tests

eba9e2b

etimberg approved these changes Nov 30, 2017

View reviewed changes

simonbrunel mentioned this pull request Dec 2, 2017

Mixed chart type bar/line : the dots are not centered in the bars #3465

Closed

simonbrunel merged commit 15d1056 into chartjs:master Dec 2, 2017

simonbrunel deleted the bar-equally-sized branch December 2, 2017 11:38

This was referenced Dec 23, 2017

Issue with x axis stacking for a stacked bar chart #5072

Closed

Incorrect bar positioning with time scale #5090

Closed

This was referenced Jan 11, 2018

Time cartesian with grouped data #5139

Closed

Bug stacked bar #5141

Closed

benmccann mentioned this pull request Jan 22, 2018

[BUG] bar charts scales are inconsistent when type: 'time' and unit: 'week' OR 'month' #3747

Closed

simonbrunel mentioned this pull request Mar 1, 2018

[BUG] Bar chart with linear time scale produces inconsistent bar width #5306

Closed

nagix mentioned this pull request Jun 26, 2018

Update the descriptions of barThickness, offsetGridLines and offset #5600

Merged

benmccann mentioned this pull request Aug 4, 2018

Flex mode in bar chart not documented #5659

Closed

nesnes mentioned this pull request Nov 16, 2018

[BUG] Regression between 2.7.1 an 2.7.2 in bar charts with timescale #5844

Closed

benmccann mentioned this pull request Jun 4, 2019

bar chart with widths based on time range #6317

Closed

snyk-bot mentioned this pull request Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement equally sized bars #4994

Implement equally sized bars #4994

simonbrunel commented Nov 26, 2017 •

edited

benmccann Nov 26, 2017

benmccann Nov 26, 2017

simonbrunel Nov 26, 2017

benmccann commented Nov 26, 2017

btumbleson commented Nov 26, 2017 •

edited

simonbrunel commented Nov 26, 2017

etimberg left a comment

etimberg Nov 26, 2017

simonbrunel Nov 27, 2017

simonbrunel commented Nov 30, 2017

simonbrunel commented Nov 30, 2017

etimberg commented Nov 30, 2017

andig commented Dec 2, 2017

simonbrunel commented Dec 2, 2017

AlGantori commented Jan 22, 2018 •

edited

theStorysEnd commented Feb 10, 2018

andig commented Feb 10, 2018

JannemanDev commented Feb 11, 2018 •

edited

Implement equally sized bars #4994

Implement equally sized bars #4994

Conversation

simonbrunel commented Nov 26, 2017 • edited

benmccann Nov 26, 2017

Choose a reason for hiding this comment

benmccann Nov 26, 2017

Choose a reason for hiding this comment

simonbrunel Nov 26, 2017

Choose a reason for hiding this comment

benmccann commented Nov 26, 2017

btumbleson commented Nov 26, 2017 • edited

simonbrunel commented Nov 26, 2017

etimberg left a comment

Choose a reason for hiding this comment

etimberg Nov 26, 2017

Choose a reason for hiding this comment

simonbrunel Nov 27, 2017

Choose a reason for hiding this comment

simonbrunel commented Nov 30, 2017

simonbrunel commented Nov 30, 2017

etimberg commented Nov 30, 2017

andig commented Dec 2, 2017

simonbrunel commented Dec 2, 2017

AlGantori commented Jan 22, 2018 • edited

theStorysEnd commented Feb 10, 2018

andig commented Feb 10, 2018

JannemanDev commented Feb 11, 2018 • edited

simonbrunel commented Nov 26, 2017 •

edited

btumbleson commented Nov 26, 2017 •

edited

AlGantori commented Jan 22, 2018 •

edited

JannemanDev commented Feb 11, 2018 •

edited