Fix for #3293 #4266

bokysan · 2017-05-17T06:36:29Z

Hello,

please find below an implementation of feature #3293.

You are now able to do the following:

options: {
    elements: {
        rectangle: {
            borderSkipped: [ 'left', 'right' ],
       }
    },
   ...
}

Example of implementation is visible here: https://bokysan.github.io/Chart.js/

Please note that the code also includes the initial "undocumented" support setting the borderWidth in the same manner, as suggested in the last comment:

options: {
    elements: {
        rectangle: {
            borderWidth: [ 0, 2, 4, 6 ],
       }
    },
   ...
}

However not all user cases have been tested -- especially with the semi-transparent colours -- so I would leave it as undocumented for the time being.

Cheers,
B

etimberg

Left some comments. @simonbrunel any comments?

etimberg · 2017-05-17T22:33:03Z

src/elements/element.rectangle.js

+			// - top-bottom + left-right
+			// - top + right + bottom + left
+			return !vm.borderWidth ? [] :
+				vm.borderWidth instanceof Array && vm.borderWidth.length === 1 ? {


[nit] could use helpers.isArray here

I would also put vm.borderWidth into a local variable to improve minification

This whole block would be more compact and efficient if borderWidth is an object.

simonbrunel

Really nice feature but I'm not sure about the public API (using arrays to defines these properties), why not using objects instead:

doesn't require to specify all values to set the left side
it works better with the config merge process
easier to test values: if (borderSkipped.left)
it's clearer what value for what side

borderSkipped: true; // shorthand for all sides
borderSkipped: {
    left: true,
    top: true
};

borderWidth: 1; // << shorthand for all sides
borderWidth: {
    left: 2,
    right: 1
};

Will need unit tests as well :)

simonbrunel · 2017-05-18T07:54:46Z

src/elements/element.rectangle.js

@@ -50,11 +50,106 @@ module.exports = function(Chart) {
 	}

 	Chart.elements.Rectangle = Chart.Element.extend({
-		draw: function() {
+		getBorderWidth: function(top, right, bottom, left) {


Should be marked @private

simonbrunel · 2017-05-18T07:55:11Z

src/elements/element.rectangle.js

+			// - top-bottom + left-right
+			// - top + right + bottom + left
+			return !vm.borderWidth ? [] :
+				vm.borderWidth instanceof Array && vm.borderWidth.length === 1 ? {


This whole block would be more compact and efficient if borderWidth is an object.

simonbrunel · 2017-05-18T07:55:22Z

src/elements/element.rectangle.js

+			// - top + right + bottom + left
+			return !vm.borderWidth ? [] :
+				vm.borderWidth instanceof Array && vm.borderWidth.length === 1 ? {
+					top: ceiling(vm.borderWidth[0], barSizeTopBottom),


Why not using Math.min(vm.borderWidth[0], barSizeLeftRight)?

simonbrunel · 2017-05-18T07:55:28Z

src/elements/element.rectangle.js

+					left: ceiling(vm.borderWidth, barSizeLeftRight)
+				};
+		},
+		drawShape: function(borders) {


Should be marked @private

simonbrunel · 2017-05-18T07:55:46Z

src/elements/element.rectangle.js

+			ctx.lineWidth = 0;
+			ctx.moveTo(borders.bottom[0][0], borders.bottom[0][1]);
+			for (var i in borders) {
+				if (borders.hasOwnProperty(i)) {


Seems counter efficient to iterate on this borders object and have all these hasOwnProperty useless calls. Since it's an internal variable, why not using an array instead and convert this for..in loop to a tradition for(var i=0; i<4; ++i) loop?

simonbrunel · 2017-05-18T07:57:19Z

src/elements/element.rectangle.js

+		},
+		adjustBorders: function(borders, borderWidth, borderSkipped) {
+			// Make sure border lengths are adjusted for border width.
+			if (!borderSkipped.includes('top') && borderWidth.top > 0) {


Array.includes is not supported by IE (though, if borderSkipped is an object, no need for that call).

simonbrunel · 2017-05-18T07:57:44Z

src/elements/element.rectangle.js

+				borders.right = [[borders.right[0][0], borders.right[0][1] + borderWidth.bottom/2], borders.right[1]];
+			}
+		},
+		drawBorders: function(borders, borderWidth, borderSkipped) {


Should be marked @private

simonbrunel · 2017-05-18T07:57:50Z

src/elements/element.rectangle.js

+
+			ctx.strokeStyle = vm.borderColor;
+			for (var i in borders) {
+				if (borders.hasOwnProperty(i)) {


Same here, iterating on this borders object doesn't seem efficient

simonbrunel · 2017-05-18T07:58:01Z

src/elements/element.rectangle.js

+						// Skip drawing the border
+						continue;
+					}
+					ctx.beginPath();


Since the strokeStyle doesn't change for each border, beginPath() and stroke() should be moved outside the loop:

ctx.strokeStyle = vm.borderColor; ctx.beginPath(); for (...) { ctx.moveTo(...) ctx.lineTo(...) } ctx.stroke();

simonbrunel · 2017-05-18T08:00:26Z

src/elements/element.rectangle.js

-				corner = cornerAt(i);
-				ctx.lineTo(corner[0], corner[1]);
-			}
+			var borders = {


For performance sake, this internal borders object should be an array of array (properly commented):

var borders = [ [x0, y0, x1, y1], //< top [x0, y0, x1, y1], //< right [x0, y0, x1, y1], //< bottom [x0, y0, x1, y1], //< left ];

bokysan · 2017-05-18T12:12:04Z

Thank you for the feedback; I'll look into optimising the code. I've been thinking borderWidth being an object though -- it does simplify things, but at the minor expense of not being able to set the top-bottom / left-right borders jointly; e.g.:

borderWidth: [ 2, 4 ]

as opposed to

borderWidth: { top: 2, right: 4, bottom: 2, left: 4 }

Seems more brief and in line with how CSS handles it. What say you - would it be better to support both syntaxes? What's the project policy on this?

simonbrunel · 2017-05-18T12:59:57Z

Arrays are evils when merging configs because they can't be merged (ref. scale configs). The CSS way to define TRBL is brief but not intuitive (at least there is alternatives, for example: border-left-width, etc). And as a consistency note: we already use the object form for the padding config.

That's why I think the first implementation should be entirely based on the object approach (with boolean/int shorthands). The rectangle element defaults should use the object form:

Chart.defaults.global.elements.rectangle = {
   borderWidth: { },   // undefined/null == 0
};

I agree that other forms are interesting, so we could add support for 't r b l' string or Array since it would just imply parsing to convert into the object form. But I would do that in another PR to avoid overloading this one and because it would required some refactor with the layout padding.

@etimberg thoughts?

bokysan · 2017-05-18T13:11:30Z

Agreed. I'm convinced.

Just occurred to me, though -- I've just followed through with bordersSkipped, however, it's a double negative. Humans usually better comprehend positive statements, e.g., instead of

bordersSkipped: {
    bottom: true
}

it would be clearer to say:

showBorders: {
    bottom: false
}

I'm not sure how that would affect compatibility with old code, though.

simonbrunel · 2017-05-18T13:28:23Z

That's a very good point and if I correctly understand this config, the answer is: no need for a new config, just deprecate borderSkipped because it's now redundant with borderWidth per side:

borderSkipped: 'bottom' == borderWidth: {bottom: 0}

or 

borderSkipped: ['left', 'bottom'] == borderWidth: {left: 0, bottom: 0}

or 

borderSkipped: {left: true, bottom: true} == borderWidth: {left: 0, bottom: 0}

So I would not add object/array support for borderSkipped but we need to maintain backward compatibility. I think we could simply ignore borderSkipped if borderWidth is not a number?

etimberg · 2017-05-18T23:27:51Z

Interesting point on deprecating borderSkipped. If we do deprecate it, we should make sure to update the default configs for vertical and horizontal bar charts

bokysan · 2017-05-19T05:48:19Z

I've done some initial refactoring -- using borderWidth instead of borderSkipped simplifies code a bit.

benmccann · 2017-07-29T19:45:51Z

@bokysan this PR needs a rebase

benmccann · 2017-11-01T03:35:52Z

@bokysan a reminder that this PR needs to be rebased

etimberg · 2017-12-31T14:51:59Z

Closing as out of date

Fix for chartjs#3293

b615e1c

etimberg reviewed May 17, 2017

View reviewed changes

simonbrunel added this to the Version 2.7 milestone May 18, 2017

simonbrunel added the type: enhancement label May 18, 2017

simonbrunel requested changes May 18, 2017

View reviewed changes

simonbrunel removed this from the Version 2.7 milestone Aug 28, 2017

etimberg closed this Dec 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #3293 #4266

Fix for #3293 #4266

bokysan commented May 17, 2017 •

edited

etimberg left a comment

etimberg May 17, 2017

etimberg May 17, 2017

simonbrunel May 18, 2017

simonbrunel left a comment

simonbrunel May 18, 2017

simonbrunel May 18, 2017

simonbrunel May 18, 2017

simonbrunel May 18, 2017

simonbrunel May 18, 2017

simonbrunel May 18, 2017

simonbrunel May 18, 2017

simonbrunel May 18, 2017

simonbrunel May 18, 2017

simonbrunel May 18, 2017

bokysan commented May 18, 2017

simonbrunel commented May 18, 2017 •

edited

bokysan commented May 18, 2017 •

edited

simonbrunel commented May 18, 2017 •

edited

etimberg commented May 18, 2017

bokysan commented May 19, 2017

benmccann commented Jul 29, 2017

benmccann commented Nov 1, 2017

etimberg commented Dec 31, 2017

Fix for #3293 #4266

Fix for #3293 #4266

Conversation

bokysan commented May 17, 2017 • edited

etimberg left a 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 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bokysan commented May 18, 2017

simonbrunel commented May 18, 2017 • edited

bokysan commented May 18, 2017 • edited

simonbrunel commented May 18, 2017 • edited

etimberg commented May 18, 2017

bokysan commented May 19, 2017

benmccann commented Jul 29, 2017

benmccann commented Nov 1, 2017

etimberg commented Dec 31, 2017

bokysan commented May 17, 2017 •

edited

simonbrunel commented May 18, 2017 •

edited

bokysan commented May 18, 2017 •

edited

simonbrunel commented May 18, 2017 •

edited