Fix and refactor bar controllers #4044

simonbrunel · 2017-03-19T18:18:25Z

Merge most of the horizontalBar controller into the bar one and fix stack groups and bar positioning when scales are stacked but also when a min and/or max tick values are explicitly defined. In addition to simplify the whole bar chart logic (which should make codeclimate happier), it decreases the minified size of about 3.7KB.

Important: this is a breaking change for derived controllers that rely on the following removed methods:

// Vertical
calculateBarWidth() -> ruler.barSize;
calculateBarBase() -> return this.calculateBarValuePixels(datasetIndex, index).base;
calculateBarX() -> return this.calculateBarIndexPixels(datasetIndex, index, ruler).center;
calculateBarY() -> return this.calculateBarValuePixels(datasetIndex, index).head;

// Horizontal
calculateBarHeight() -> return ruler.barSize;
calculateBarBase() -> return this.calculateBarValuePixels(datasetIndex, index).base;
calculateBarX() -> return this.calculateBarValuePixels(datasetIndex, index).head;
calculateBarY() -> return this.calculateBarIndexPixels(datasetIndex, index, ruler).base;

simonbrunel · 2017-03-19T18:21:37Z

@potatopeelings you may be interested to review these changes since it impacts the stacking logic.

simonbrunel · 2017-03-19T18:36:38Z

src/controllers/controller.bar.js

+		 * @deprecated
+		 * @private
+		 */
+		calculateBarWidth: function(ruler) {


@etimberg I'm not sure I would keep these internal deprecated calculateBar* methods: they are not called anymore by the new implementation and most of the time people might have overridden them (which will obviously not work). The new implementation is more optimized and trying to keep compatibility will be slower (and dirty), so I don't think it worth the hack for (a few?) custom implementations using private code (e.g #2726, #3858).

What do you think?

/cc @daviddevore11 @yoerivdm

Right, if they are overridden they won't be called anymore. I think the recommended way to achieve the old behaviour would be to override updateElement to set the properties as expect. I am fine to remove these since they won't do anything and overridding them won't work. We should wait to do anything until we see what users say

etimberg · 2017-03-19T18:59:25Z

src/controllers/controller.bar.js

+				borderSkipped: custom.borderSkipped ? custom.borderSkipped : rectangleOptions.borderSkipped,
+				backgroundColor: custom.backgroundColor ? custom.backgroundColor : helpers.getValueAtIndexOrDefault(dataset.backgroundColor, index, rectangleOptions.backgroundColor),
+				borderColor: custom.borderColor ? custom.borderColor : helpers.getValueAtIndexOrDefault(dataset.borderColor, index, rectangleOptions.borderColor),
+				borderWidth: custom.borderWidth ? custom.borderWidth : helpers.getValueAtIndexOrDefault(dataset.borderWidth, index, rectangleOptions.borderWidth)


This check would have failed in the old code for custom.borderWidth === 0? Are we ok with keeping it now? Should we fix it in another PR?

I would go for another PR, maybe the one supporting functors for these data options.

Ok, sounds good

etimberg · 2017-03-19T19:06:20Z

src/controllers/controller.bar.js

+		 * @deprecated
+		 * @private
+		 */
+		calculateBarWidth: function(ruler) {


Right, if they are overridden they won't be called anymore. I think the recommended way to achieve the old behaviour would be to override updateElement to set the properties as expect. I am fine to remove these since they won't do anything and overridding them won't work. We should wait to do anything until we see what users say

etimberg · 2017-03-19T19:07:39Z

test/specs/controller.bar.tests.js

-			{b: 290, w: 83, x: 202, y: 419},
-			{b: 290, w: 83, x: 318, y: 161},
-			{b: 290, w: 83, x: 434, y: 419}
+			{b: 290, w: 83 / 2, x: 63, y: 161},


previously these bars were the same full width?

I think that was a bug introduced by #3563, it should be half the width because the x scale is not stacked, right?

Yup, just confirming the change 😄

Merge most of the horizontalBar controller into the bar one but also fix stack groups and bar positioning when scales are stacked or when a min and/or max tick values are explicitly defined. Note that this is a breaking change for derived controllers that rely on the following removed methods: `calculateBarBase`, `calculateBarX`, `calculateBarY`, `calculateBarWidth` and `calculateBarHeight`.

simonbrunel · 2017-03-25T10:09:05Z

@etimberg PR rebased, I also removed the deprecated methods and updated the PR description. May be safer that you checkout and test my branch to confirm that it's good to be merged.

Final minified size improvement: 3.7KB :)

potatopeelings · 2017-03-25T14:20:29Z

@simonbrunel - looks great! Love the way stackIndex and stackCount methods were merged :-)

potatopeelings · 2017-03-25T14:32:21Z

src/controllers/controller.bar.js

+			var vscale = me.getValueScale();
+			var base = vscale.getBasePixel();
+			var horizontal = vscale.isHorizontal();
+			var ruler = me._ruler || me.getRuler();


I assume this || me.getRuler() is there because of addElementAndReset calling updateElement - would moving line 94 to updateElement and injecting ruler into updateElementGeometry instead make that clearer?

I didn't even notice that update() wasn't called in certain cases, good point! Your suggestion was my first implementation but I did this || me.getRuler() in updateElementGeometry in case update() or updateElement() has been overridden, in which case it will still build the ruler (having it built only once per update is a great optimization). Though, if update() has been overridden, the ruler will be created but never updated.

Um... if updateElementGeometry is private, the two ways it can get called if updateElement is overridden are

Someone uses the private updateElementGeometry method in a plugin or custom override (not expecting it to work across version changes)

One of the existing bar methods uses it in future (none do right now)

So, we want to handle one / both of these possibilities?

And thinking back on my comment, I'm ok with its current position at line 94 - we can easily change it in future easily (if needed) since the method is private :-)

etimberg · 2017-03-25T15:14:30Z

@simonbrunel I tested it out and didn't notice anything :)

simonbrunel added the type: bug label Mar 19, 2017

simonbrunel added this to the Version 2.6 milestone Mar 19, 2017

simonbrunel requested review from etimberg and panzarino March 19, 2017 18:18

simonbrunel commented Mar 19, 2017

View reviewed changes

etimberg reviewed Mar 19, 2017

View reviewed changes

simonbrunel mentioned this pull request Mar 24, 2017

Group stacked bar charts (#2643) #3563

Merged

etimberg added the breaking change label Mar 24, 2017

simonbrunel force-pushed the bar-refactor branch from bd2b0e1 to 8a2f462 Compare March 25, 2017 09:52

simonbrunel force-pushed the bar-refactor branch from 8a2f462 to 38a0f64 Compare March 25, 2017 10:02

potatopeelings reviewed Mar 25, 2017

View reviewed changes

simonbrunel mentioned this pull request Apr 7, 2017

Floating bar charts #4120

Closed

etimberg approved these changes Apr 8, 2017

View reviewed changes

etimberg merged commit 50a80da into chartjs:master Apr 8, 2017

simonbrunel deleted the bar-refactor branch April 14, 2017 07:52

jipiboily mentioned this pull request Jan 21, 2019

Update to chart.js 2.6? chartjs/Chart.BarFunnel.js#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and refactor bar controllers #4044

Fix and refactor bar controllers #4044

simonbrunel commented Mar 19, 2017 •

edited

Loading

simonbrunel commented Mar 19, 2017

simonbrunel Mar 19, 2017

etimberg Mar 19, 2017

etimberg Mar 19, 2017

simonbrunel Mar 19, 2017

etimberg Mar 19, 2017

etimberg Mar 19, 2017

etimberg Mar 19, 2017

simonbrunel Mar 19, 2017

etimberg Mar 19, 2017

simonbrunel commented Mar 25, 2017 •

edited

Loading

potatopeelings commented Mar 25, 2017

potatopeelings Mar 25, 2017 •

edited

Loading

simonbrunel Mar 25, 2017

potatopeelings Mar 26, 2017

etimberg commented Mar 25, 2017

Fix and refactor bar controllers #4044

Fix and refactor bar controllers #4044

Conversation

simonbrunel commented Mar 19, 2017 • edited Loading

simonbrunel commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel commented Mar 25, 2017 • edited Loading

potatopeelings commented Mar 25, 2017

potatopeelings Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etimberg commented Mar 25, 2017

simonbrunel commented Mar 19, 2017 •

edited

Loading

simonbrunel commented Mar 25, 2017 •

edited

Loading

potatopeelings Mar 25, 2017 •

edited

Loading