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

Fixes bug #5289: Bars do not appear at correct X axis position when specified in {x, y} format #5298

Closed
wants to merge 8 commits into from
Closed
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 64 additions & 2 deletions src/core/core.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,11 @@ module.exports = function(Chart) {
if (!dataset._meta) {
dataset._meta = {};
}

if (!dataset.data) {
dataset.data = [];
}
// format the data if the data array is missing some point according to the labels
dataset.data = me.formatDataset(dataset.data);
var meta = dataset._meta[me.id];
if (!meta) {
meta = dataset._meta[me.id] = {
Expand Down Expand Up @@ -933,7 +937,65 @@ module.exports = function(Chart) {
me.lastActive = me.active;

return changed;
}
},
/**
* format the data list if the data is missing some points in the datasets.
Copy link
Contributor

Choose a reason for hiding this comment

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

The first letter of a method doc is typically capitalized. Same below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Capitalized all the first letter of the method doc.

* @private
* @param {array} dataArray array to format
* @return {array} the formated array
*/
formatDataset: function(dataArray) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private functions should be prefixed with _

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The prefixed is added.
Pls check.

var labels = this.chart.data.labels;
var tmp = dataArray.slice(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

there's no reason to create tmp here as a copy of dataArray. the only actions taken on it are read-only. just use dataArray directly and remove tmp

var labelLen = labels.length;
Copy link
Contributor

@benmccann benmccann May 20, 2018

Choose a reason for hiding this comment

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

there's no need to create a variable for this value since it's only used once. same with dataLen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the labelLen and dataLen variables since they are used only once.

var dataLen = dataArray.length;
var result = dataArray;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is where you should make the defensive copy. don't modify the input to the function. instead do var result = dataArray.slice(0); // create copy of input

var match = this.dataInLabel(tmp, labels);
if (match && dataLen < labelLen) {
for (var i = 0; i < labels.length; i++) {
var label = labels[i];
result[i] = {x: label, y: this.getY(label, tmp)};
}
}
return result;
},
/**
* return the y data of the label, if no this label, return null
Copy link
Contributor

Choose a reason for hiding this comment

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

The spacing is a bit off on the doc for this comment and the next one.

They should probably be more like the first method doc you have like:

/**
 *

This one is missing a space:

/**
*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The missing space is added now in all the places.

* @private
* @param {string} label the label in the labels array
* @param {array} dataArray the data in the datasets.
* @return {number} the y data according to the label
*/
getY: function(label, dataArray) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be put with the other functions (line 25-68) since it doesn't use any class members

var y = null;
for (var i = 0; i < dataArray.length; i++) {
var item = dataArray[i];
if (item.x === label) {
y = item.y;
}
}
return y;
},
/**
* find if the data in datasets is existed in the labels
Copy link
Contributor

Choose a reason for hiding this comment

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

"is existed" -> "exists"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

* @private
* @param {array} dataArray the data in the datasets
* @param {array} labelArray the label in the labels array
* @return {boolean} the match result, true is included
*/
dataInLabel: function(dataArray, labelArray) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be put with the other functions (line 25-68) since it doesn't use any class members

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have put these two functions on the other place.

var result = false;
for (var i = 0; i < dataArray.length; i++) {
var item = dataArray[i];
if (item === null || item === undefined || item.x === undefined) {
break;
}
if (labelArray.indexOf(item.x) > -1) {
result = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

just return true here. you don't need the result var and can return false as the last line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as you required.

}
}
return result;
},
});

/**
Expand Down