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

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

Closed
wants to merge 4 commits into from

Conversation

Zamaroth
Copy link

@Zamaroth Zamaroth commented Apr 6, 2017

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

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.

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.

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

if (optionTicks.display) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@Zamaroth Zamaroth Apr 7, 2017

Choose a reason for hiding this comment

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

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
Copy link
Author

Zamaroth commented Apr 7, 2017

Made a jsfiddle.

}

return {
id: 'gridLines',
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

etimberg commented Jun 4, 2017

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

@etimberg
Copy link
Member

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

@Zamaroth
Copy link
Author

I´ll take a look at it

@simonbrunel
Copy link
Member

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

@Zamaroth
Copy link
Author

Zamaroth commented Jul 7, 2017

I rebased the branch against latest master and updated the jsFiddle

@andig
Copy link
Contributor

andig commented Aug 11, 2017

@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
Copy link
Author

Zamaroth commented Aug 13, 2017

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

@andig
Copy link
Contributor

andig commented Aug 14, 2017

I refactored the code, rebased the branch against latest master

min/max is now honored as well- thanks!

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

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

'use strict';

module.exports = function(Chart) {
var helpers = Chart.helpers;
Copy link
Member

Choose a reason for hiding this comment

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

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


var lineWidth, lineColor, borderDash, borderDashOffset;

if (tickIndex === (typeof scale.zeroLineIndex !== 'undefined' ? scale.zeroLineIndex : 0)) {
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 should honor the zero line only if explicitly defined by the scale:

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

Copy link
Author

@Zamaroth Zamaroth Aug 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

zeroLineColor? do you mean zeroLineIndex?

Copy link
Author

Choose a reason for hiding this comment

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

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

var globalDefaults = Chart.defaults.global;

// Stores all gridLines that should be displayed
var lines = [];
Copy link
Member

Choose a reason for hiding this comment

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

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.

}

function collectVisibleGridLines(chart) {
lines = [];
Copy link
Member

Choose a reason for hiding this comment

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

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

var glHashByOrientantion = gridLinesHash[scale.isHorizontal() ? 'horizontal' : 'vertical'];
var position;

for (var tickIndex = 0; tickIndex < scale.ticks.length; tickIndex++) {
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

you mean where the scaleOptions is declared?

Copy link
Member

Choose a reason for hiding this comment

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

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;

    //...
});

id: 'gridlines',

afterUpdate: function(chart) {
collectVisibleGridLines(chart);
Copy link
Member

Choose a reason for hiding this comment

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

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);
            }
        }
    };
}

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

if (optionTicks.display) {
if (optionTicks.display && itemToDraw.label !== undefined && itemToDraw.label !== '') {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@simonbrunel simonbrunel Aug 14, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@benmccann
Copy link
Contributor

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

@Zamaroth
Copy link
Author

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
Copy link
Author

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


// 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
Copy link
Author

Choose a reason for hiding this comment

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

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

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.

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

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

Choose a reason for hiding this comment

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

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

// 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'];
Copy link
Member

Choose a reason for hiding this comment

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

[nit] orientation has a typo in it

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

Choose a reason for hiding this comment

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

[nit] same here


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])) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so

@etimberg
Copy link
Member

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

@benmccann
Copy link
Contributor

@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
Copy link
Contributor

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

@okcoker
Copy link

okcoker commented May 8, 2018

Did the rebase for this here #5480

@benmccann
Copy link
Contributor

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

@okcoker
Copy link

okcoker commented Aug 3, 2018

@benmccann need anything from me here?

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.

6 participants