Move drawing of gridLines into separate plugin and add border functionality #4117

Zamaroth · 2017-04-06T14:03:16Z

This pull request is an extended version of pull request #4058, discussed directly with @etimberg .

Summary:

Created a separate plugin for drawing of gridLines and moved them outside of core.scale.js to allow better control over them and interaction between them, solving multiple problems regarding the gridLines.
Also added the border functionality to allow separate configuration of axisLine discussed in issue #4041

Solved problems:

Overlapping gridLines: when having multiple axes on the same side of the graph, multiple gridLines were drawn on top of each other causing overlapping and merging of multiple colors with alphas.
Inconsistent behaviour: axisLine(the one near ticks) of non-adjacement axes to the chartArea could be either hidden, or visible with the color of gridLines depending on the scale.gridLines.drawBorder option.
However the axisLine of an adjacement axis could never be completely hidden because of the overlapping of gridLines.
Also the axisLine color was the same as gridLines of that axis, but it has the opposite direction. For better explanation check Coloring an axis line separately from gridLines #4041
Ignoring the offsetGridLines option: when offsetGridLines option is enabled there is one less tick label than there should be tick marks and gridLines. Iterating through ticks array without respecting the option was resulting in one missing gridLine and tick mark

Added functionality:

Borders: there is an option to enable border on any axis resulting in the ability to set following border options: color, lineWidth, borderDash, borderDashOffset

I havent updated the documentation yet, but I want to start the pull request already as there is quite a lot to review and update the documentation while it is being reviewed.
Also please ignore the first two commits by Cizmarik as his pull request was already merged yesterday with those changes. Also all the merge commits as they were just the result of updating our fork, caused by us being new and a bit confused with this whole git thing :)

etimberg

Overall I think this is looking pretty good. Is it possible to see a test fiddle of it in action to play with? I think it might be a good idea to add a comment that explains what an "undefined" border is. It will help when others look at the code.

etimberg · 2017-04-06T23:18:03Z

src/core/core.scale.js

 					context.stroke();
 					context.restore();
 				}

-				if (optionTicks.display) {


Might make sense to do this for any value of itemToDraw.label that isn't truthy. Blank labels don't need to be drawn and same with null labels. What do you think?

items with null or undefined label aren't pushed into the array at all, thanks to this statement.
The blank labels don't need to be drawn tho. The question is if they should be skipped the same way as when it's null(its ticks and gridLines wont be drawn) or in the statement you linked so there will be a blank tick, but the label itself wont be drawn.

Hmmm, for grid lines we have the following logic:
if label is a string, draw label, tick and grid line
if label is null or undefined hide the label, tick and gridline.

This allows users to filter out grid lines by return null from the tick callback. here is an example of filtering

i understand, i'll add just add the statement to skip the drawing of label itself if it's empty string then

Sounds good

etimberg · 2017-04-06T23:40:15Z

src/plugins/plugin.gridLines.js

+		// Draws all the borders. Skips undefined orders which would be overlapped by a defined border
+		helpers.each(bordersToDraw, function(borderToDraw) {
+			// When the border is undefined, check if there isn't a defined border on the same place, if there is one dont draw this border
+			if (!borderToDraw.undefinedBorder || bordersToDraw.findIndex(bordersOverlap, borderToDraw) === -1) {


I realize this might not have good performance, but I think it's fine given that the number of axes is usually small

there will always be numberOfAxes+4 number of items in that array, so as you said it should not really have any impact on performance

Zamaroth · 2017-04-07T15:04:43Z

Made a jsfiddle.

simonbrunel · 2017-04-22T13:23:13Z

src/plugins/plugin.gridLines.js

+	}
+
+	return {
+		id: 'gridLines',


I still need to fully review that PR but a first feedback according the plugin guidelines: the id should be lowercase (id: 'gridlines'). Can you also rename the file to: src/plugins/plugin.gridlines.js?

etimberg · 2017-06-04T14:14:20Z

@simonbrunel have you had a chance to review this further?

etimberg · 2017-06-24T17:11:33Z

@Zamaroth could you rebase this agains the latest changes in master?

Zamaroth · 2017-06-28T20:14:01Z

I´ll take a look at it

simonbrunel · 2017-07-07T14:26:30Z

@etimberg not yet, will try as soon as I can

Zamaroth · 2017-07-07T14:57:49Z

I rebased the branch against latest master and updated the jsFiddle

andig · 2017-08-11T15:23:25Z

@Zamaroth here's one test case for this pr: http://jsfiddle.net/andig2/vLgosao7/ ressources copied from your fiddle https://jsfiddle.net/y0whvnvk/4/

One issue I'm seeing is that it doesn't respect min/max settings on the x axis (recently fixed in #4522), maybe the fiddle is not yet using your recent rebased version?

Zamaroth · 2017-08-13T22:11:22Z

I refactored the code, rebased the branch against latest master and updated the jsFiddle.
I didn't update the docs yet.

andig · 2017-08-14T07:21:13Z

I refactored the code, rebased the branch against latest master

min/max is now honored as well- thanks!

simonbrunel

Thanks @Zamaroth, can you rebase and resolve conflicts. We still need to figure out if that PR introduces behavior/breaking changes before merging.

simonbrunel · 2017-08-14T08:14:39Z

src/plugins/plugin.gridlines.js

+'use strict';
+
+module.exports = function(Chart) {
+	var helpers = Chart.helpers;


We changed the way how to require dependencies (see this code)

simonbrunel · 2017-08-14T08:16:14Z

src/plugins/plugin.gridlines.js

+
+		var lineWidth, lineColor, borderDash, borderDashOffset;
+
+		if (tickIndex === (typeof scale.zeroLineIndex !== 'undefined' ? scale.zeroLineIndex : 0)) {


I think we should honor the zero line only if explicitly defined by the scale:

if (tickIndex === scale.zeroLineIndex) { // ... }

im gonna change this statement to tickIndex !== undefined && tickIndex === scale.zeroLineIndex, because i send undefined to tickIndex when getting the last extra gridLine when offsetGridLines is enabled...
link

zeroLineColor? do you mean zeroLineIndex?

yes, i meant zeroLineIndex... already corrected the comment

simonbrunel · 2017-08-14T08:19:08Z

src/plugins/plugin.gridlines.js

+	var globalDefaults = Chart.defaults.global;
+
+	// Stores all gridLines that should be displayed
+	var lines = [];


A plugin is potentially called for many charts, so it's not safe to assume that the update and draw will be called synchronously per chart (especially if there is animations). This lines array would contain lines for different charts.

simonbrunel · 2017-08-14T08:19:31Z

src/plugins/plugin.gridlines.js

+	}
+
+	function collectVisibleGridLines(chart) {
+		lines = [];


This method should build a local lines array and return it.

simonbrunel · 2017-08-14T08:21:41Z

src/plugins/plugin.gridlines.js

+				var glHashByOrientantion = gridLinesHash[scale.isHorizontal() ? 'horizontal' : 'vertical'];
+				var position;
+
+				for (var tickIndex = 0; tickIndex < scale.ticks.length; tickIndex++) {


Can you gather all var declarations at the top of the current scope (which is the each callback)? But also cache the loop end condition: for (i = 0, ilen = scale.ticks.length; i < ilen; ++i)

you mean where the scaleOptions is declared?

Yes, you should try to gather vars in the same scope this way to help minification.

helpers.each(chart.scales, function(scale) { var scaleOptions = scale.options; var glHashByOrientantion, position, i, ilen, tick; //... });

simonbrunel · 2017-08-14T08:37:21Z

src/plugins/plugin.gridlines.js

+		id: 'gridlines',
+
+		afterUpdate: function(chart) {
+			collectVisibleGridLines(chart);


You need to store the lines per chart and a good way to do that is to add an plugin specific private model under chart (see this example).

var MODEL_KEY = '$gridlines'; module.exports = function(Chart) { // ... return { afterUpdate: function(chart) { chart[MODEL_KEY] = { lines: collectVisibleGridLines(chart); }; } beforeDraw: function(chart) { var model = chart[MODEL_KEY]; if (model) { drawGridLines(chart.ctx, model.lines); } } }; }

simonbrunel · 2017-08-14T08:42:06Z

src/core/core.scale.js

 					context.stroke();
 					context.restore();
 				}

-				if (optionTicks.display) {
+				if (optionTicks.display && itemToDraw.label !== undefined && itemToDraw.label !== '') {


&& itemToDraw.label will test against all empty values.

simonbrunel · 2017-08-14T08:43:44Z

src/core/core.scale.js

+					tmWidth: lineWidth,
+					tmColor: lineColor,
+					tmBorderDash: borderDash,
+					tmBorderDashOffset: borderDashOffset,
 					rotation: -1 * labelRotationRadians,
 					label: label,


I would force a conversion of label to string: label: '' + label, then we can test if (label) for empty value.

simonbrunel · 2017-08-14T08:44:49Z

src/core/core.scale.js

-				var y1 = me.top;
-				var y2 = me.bottom;
+			// gridLines.drawBorder is deprecated
+			if (gridLines.drawBorder && options.borderColor !== null && options.borderWidth !== 0 && options.borderWidth !== null) {


if (gridLines.drawBorder && options.borderColor && options.borderWidth) { should be enough, right?

we discussed that borderWidth === 0 should disable the border and it should not be drawn. Thats why I check for it here. Its also checked while collecting the gridLines to determine if it should be added to the hash

if (options.borderWidth) is the same as options.borderWidth !== 0 && options.borderWidth !== null but also includes undefined, false and '', which I think are also values that are supposed to remove the grid lines?

oh I see... then what you suggested should be enough

simonbrunel · 2017-08-14T08:50:23Z

src/core/core.scale.js

+					tx2: !isHorizontal ? xTickEnd : chartArea.right + aliasPixel,
+					ty2: !isHorizontal ? chartArea.bottom + aliasPixel : yTickEnd,
+					tmWidth: gridLines.lineWidth,
+					tmColor: gridLines.color,


This will not work if gridLines.lineWidth or gridLines.color is an array

benmccann · 2017-09-28T02:46:40Z

@Zamaroth would you be able to rebase and address Simon's comments?

Zamaroth · 2017-09-28T07:29:34Z

I can. I actually adressed the comments(only locally), but i had a problem with issue #4545, which has been already merged few days after the comment. It removed logic of offsetting gridLines when offsetGridLines is enabled from getPixelForTick and moved it to getLineValue on core.scale.js, which if I remember correctly cant use from inside the plugin.
I discussed it shortly with @simonbrunel directly on slack, but didn't get an answer on what I should do with it, if it should be moved elsewhere or something else...

Zamaroth · 2017-10-21T10:38:58Z

I adressed the comments, rebased the branch against latest master and updated the jsFiddle

Zamaroth · 2017-10-21T10:40:50Z

src/plugins/plugin.gridlines.js


-	// Stores all gridLines that should be displayed
-	var lines = [];
+// This is copied from core.scale.js, which is also required here and im not sure where it should be placed for both of them to access it


This should not be here, but i dont know where to move it for both core.scale.js and this plugin to access it

etimberg

Looked through the code. I haven't run this yet

etimberg · 2017-10-21T12:48:24Z

src/core/core.scale.js

 				if (isHorizontal) {
 					y1 = y2 = options.position === 'top' ? me.bottom : me.top;
-					y1 += aliasPixel;
-					y2 += aliasPixel;
+					y1 += helpers.aliasPixel(context.lineWidth);


I would put the var aliasPixel = helpers.aliasPixel(context.lineWidth) back in since it minifies better

etimberg · 2017-10-21T13:11:10Z

src/plugins/plugin.gridlines.js

+		// Marks scale border positions to prevent overlapping of gridLines and scale borders
+		helpers.each(chart.scales, function(scale) {
+			var scaleOptions = scale.options;
+			var glHashByOrientantion = gridLinesHash[!scale.isHorizontal() ? 'horizontal' : 'vertical'];


[nit] orientation has a typo in it

etimberg · 2017-10-21T13:11:32Z

src/plugins/plugin.gridlines.js

+		// Collects gridLines
+		helpers.each(chart.scales, function(scale) {
+			var scaleOptions = scale.options;
+			var glHashByOrientantion = gridLinesHash[scale.isHorizontal() ? 'horizontal' : 'vertical'];


[nit] same here

etimberg · 2017-10-21T13:12:33Z

src/plugins/plugin.gridlines.js

+
+			if (scaleOptions.display && scaleOptions.gridLines.display && scaleOptions.gridLines.drawOnChartArea) {
+				for (var tickIndex = 0, ticksCount = scale.ticks.length; tickIndex < ticksCount; tickIndex++) {
+					if (helpers.isNullOrUndef(scale.ticks[tickIndex])) {


@simonbrunel @benmccann does this check make sense in the context of how ticks changed in 2.7.0?

etimberg · 2017-12-31T14:50:10Z

@simonbrunel have you had a chance to look at this since it was rebased?

benmccann · 2018-01-26T04:33:44Z

@Zamaroth sorry that this PR has been open for so long :-( I'm afraid it will need to be rebased again.

@simonbrunel thoughts on this one?

benmccann · 2018-04-24T16:26:50Z

@Zamaroth just a quick reminder to rebase this PR when you get a chance

okcoker · 2018-05-08T23:22:29Z

Did the rebase for this here #5480

benmccann · 2018-08-03T04:58:52Z

I'm going to close this PR as inactive in favor of the rebased PR that @okcoker opened

okcoker · 2018-08-03T07:04:30Z

@benmccann need anything from me here?

Zamaroth mentioned this pull request Apr 6, 2017

Coloring an axis line separately from gridLines #4058

Closed

etimberg reviewed Apr 6, 2017

View reviewed changes

etimberg requested review from simonbrunel and panzarino April 6, 2017 23:48

simonbrunel added this to the Version 2.7 milestone Apr 22, 2017

simonbrunel added the type: enhancement label Apr 22, 2017

simonbrunel requested changes Apr 22, 2017

View reviewed changes

etimberg mentioned this pull request Jul 1, 2017

Auto skip last #4399

Closed

Zamaroth force-pushed the t-5001 branch from d864a47 to 832f2fa Compare July 7, 2017 13:48

simonbrunel mentioned this pull request Aug 11, 2017

Feature: Make ranges for tick auto generation configurable #4644

Closed

Zamaroth force-pushed the t-5001 branch from a2ad4d5 to 90aa222 Compare August 13, 2017 21:52

simonbrunel requested changes Aug 14, 2017

View reviewed changes

simonbrunel requested a review from etimberg August 14, 2017 08:56

simonbrunel modified the milestones: Version 2.8, Version 2.7 Aug 23, 2017

simonbrunel mentioned this pull request Aug 25, 2017

Fix wild special zero grid line when undefined #4700

Merged

Zamaroth added 3 commits October 21, 2017 12:23

Create gridLines plugin

8b1c940

Refactor plugin.gridlines.js

5598a8f

Adress comments

eb094f5

Zamaroth force-pushed the t-5001 branch from 90aa222 to eb094f5 Compare October 21, 2017 10:26

Zamaroth commented Oct 21, 2017

View reviewed changes

etimberg reviewed Oct 21, 2017

View reviewed changes

Fix typos and add borderAliasPixel variable

3523f9b

Zamaroth force-pushed the t-5001 branch from d04af25 to 3523f9b Compare November 2, 2017 10:09

simonbrunel mentioned this pull request Nov 15, 2017

changing color only for gridLines at index zero #4948

Closed

benmccann mentioned this pull request Nov 16, 2017

issue #4948 #4951

Closed

simonbrunel modified the milestones: Version 2.7.2, Version 2.8 Jan 13, 2018

etimberg mentioned this pull request Jan 26, 2018

Don't draw tick across axis/border #5178

Merged

etimberg mentioned this pull request Mar 3, 2018

[Possible Bug] Bar Chart doesn't display right border #3804

Closed

okcoker mentioned this pull request May 8, 2018

(Rebase) Move drawing of gridLines into separate plugin and add border functionality #5480

Closed

benmccann closed this Aug 3, 2018

benmccann added the status: stale label Aug 3, 2018

simonbrunel removed this from the Version 2.7.3 milestone Oct 15, 2018

Zamaroth mentioned this pull request Jun 6, 2019

Coloring an axis line separately from gridLines #4041

Closed


		var lineWidth, lineColor, borderDash, borderDashOffset;

		if (tickIndex === (typeof scale.zeroLineIndex !== 'undefined' ? scale.zeroLineIndex : 0)) {

Move drawing of gridLines into separate plugin and add border functionality #4117

Move drawing of gridLines into separate plugin and add border functionality #4117

Conversation

Zamaroth commented Apr 6, 2017

Summary:

Solved problems:

Added functionality:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zamaroth Apr 7, 2017 • edited

Choose a reason for hiding this comment

Zamaroth commented Apr 7, 2017 • edited

Choose a reason for hiding this comment

etimberg commented Jun 4, 2017

etimberg commented Jun 24, 2017

Zamaroth commented Jun 28, 2017

simonbrunel commented Jul 7, 2017

Zamaroth commented Jul 7, 2017

andig commented Aug 11, 2017

Zamaroth commented Aug 13, 2017 • edited

andig commented Aug 14, 2017

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

Zamaroth Aug 14, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel Aug 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann commented Sep 28, 2017

Zamaroth commented Sep 28, 2017

Zamaroth commented Oct 21, 2017

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etimberg commented Dec 31, 2017

benmccann commented Jan 26, 2018

benmccann commented Apr 24, 2018

okcoker commented May 8, 2018

benmccann commented Aug 3, 2018

okcoker commented Aug 3, 2018

Zamaroth Apr 7, 2017 •

edited

Zamaroth commented Apr 7, 2017 •

edited

Zamaroth commented Aug 13, 2017 •

edited

Zamaroth Aug 14, 2017 •

edited

simonbrunel Aug 14, 2017 •

edited