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

tfwww · 2018-02-24T10:15:29Z

Fix #5289 by formatting the data if missing some points.

benmccann · 2018-05-20T05:13:26Z

src/core/core.controller.js

+			return result;
+		},
+		/**
+		* return the y data of the label, if no this label, return null


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:

/** *

The missing space is added now in all the places.

benmccann · 2018-05-20T05:14:36Z

src/core/core.controller.js

+					break;
+				}
+				if (labelArray.indexOf(item.x) > -1) {
+					result = true;


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

Changed as you required.

benmccann · 2018-05-20T05:19:11Z

src/core/core.controller.js

+		formatDataset: function(dataArray) {
+			var labels = this.chart.data.labels;
+			var tmp = dataArray.slice(0);
+			var labelLen = labels.length;


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

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

benmccann · 2018-05-20T05:20:33Z

src/core/core.controller.js

+			return y;
+		},
+		/**
+		* find if the data in datasets is existed in the labels


"is existed" -> "exists"

benmccann · 2018-05-20T05:21:47Z

src/core/core.controller.js

-		}
+		},
+		/**
+		 * format the data list if the data is missing some points in the datasets.


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

Capitalized all the first letter of the method doc.

benmccann · 2018-05-21T02:41:02Z

src/core/core.controller.js

+		 * @param {array} dataArray the data in the datasets.
+		 * @return {number} the y data according to the label
+		 */
+		getY: function(label, dataArray) {


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

benmccann · 2018-05-21T02:41:20Z

src/core/core.controller.js

+		 * @param {array} labelArray the label in the labels array
+		 * @return {boolean} the match result, true is included
+		 */
+		dataInLabel: function(dataArray, labelArray) {


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

I have put these two functions on the other place.

benmccann · 2018-05-21T02:44:40Z

src/core/core.controller.js

+		 * @param {array} dataArray array to format
+		 * @return {array} the formated array
+		 */
+		formatDataset: function(dataArray) {


private functions should be prefixed with _

The prefixed is added.
Pls check.

benmccann · 2018-05-23T15:46:13Z

src/core/core.controller.js

+		for (var i = 0; i < dataArray.length; i++) {
+			var item = dataArray[i];
+			if (item.x === label) {
+				y = item.y;


you can just return item.y here, which will be less code and more efficient. then remove the y variable and return null at the end if you didn't hit this

The code is changed as you required.

benmccann · 2018-06-15T16:04:28Z

src/core/core.controller.js

+			if (item === null || item === undefined || item.x === undefined) {
+				break;
+			}
+			if (labelArray.indexOf(item.x) > -1) {


this will return true if a single item matches. is that what you want?

if you want to see if all items match then you could return false if < 0 here and return true at the end of the function

benmccann · 2018-06-15T16:08:53Z

src/core/core.controller.js

+		 */
+		_formatDataset: function(dataArray) {
+			var labels = this.chart.data.labels;
+			var tmp = dataArray.slice(0);


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

benmccann · 2018-06-15T16:10:37Z

src/core/core.controller.js

+		_formatDataset: function(dataArray) {
+			var labels = this.chart.data.labels;
+			var tmp = dataArray.slice(0);
+			var result = dataArray;


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

benmccann · 2018-07-07T19:10:58Z

@wmzhong don't forget there are some comments on this PR

benmccann · 2018-07-30T03:50:51Z

@wmzhong a quick reminder that there are some outstanding comments on this PR

benmccann · 2018-08-03T05:06:02Z

@wmzhong a reminder to take another look at this PR when you get a chance

andig · 2018-08-08T16:53:46Z

That would be a shame. But when the first review takes three months interest might have shifted 😔

benmccann · 2018-08-08T17:11:57Z

src/core/core.controller.js

+			if (match && dataArray.length < labels.length) {
+				for (var i = 0; i < labels.length; i++) {
+					var label = labels[i];
+					result[i] = {x: label, y: getY(label, tmp)};


I think you need to check if it's a horizontal or vertical chart here and can't just assume that the label will be on the x-axis

And getY should probably be called getValue and also do the same check

benmccann · 2018-08-08T17:15:02Z

Yeah, can definitely understand that the long review cycles are frustrating. We'd love help reviewing PRs if you're able to go through the pending PRs and help review.

To be clear, if we close any PR due to inactivity, it's always welcome to be re-opened in the future when the feedback is ready to be addressed. The main reason for closing inactive PRs is to help make it clear which PRs need reviews so that we can more efficiently get through them and try to cut down the review cycle

benmccann · 2018-08-15T13:58:20Z

@wmzhong I really appreciate the contribution and I would like to see it make its way in, but it has been quite some time without a response. I'm going to close this PR for now, but please do update it and let us know when you do so that we can reopen it and finish getting it reviewed and merged. Sorry once again for the delay on the original review

wmzhong added 4 commits February 24, 2018 16:39

fix the data missing bar bug.

7a4ba6b

adjust the gulp lint

d1809f7

adjust the unit test

e387285

add the comment

98ec449

tfwww changed the title ~~Fixes Bars do not appear at correct X axis position when specified in {x, y} format #5289~~ Fixes bug #5289: Bars do not appear at correct X axis position when specified in {x, y} format Feb 24, 2018

benmccann reviewed May 20, 2018

View reviewed changes

wmzhong added 2 commits May 21, 2018 10:10

fix the typo as required.

996c6af

fix the space tab mix issue.

740e0bb

benmccann reviewed May 21, 2018

View reviewed changes

put some funs to other place

5d9dc40

benmccann reviewed May 23, 2018

View reviewed changes

make less code

4ab606c

benmccann reviewed Jun 15, 2018

View reviewed changes

benmccann mentioned this pull request Jun 17, 2018

Issue with x axis stacking for a stacked bar chart #5072

Closed

benmccann mentioned this pull request Jul 1, 2018

Allow specifying bar chart via xy data points #4565

Merged

benmccann added the status: stale label Jul 30, 2018

benmccann reviewed Aug 8, 2018

View reviewed changes

benmccann closed this Aug 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tfwww commented Feb 24, 2018 •

edited

Loading

benmccann May 20, 2018

tfwww May 21, 2018

benmccann May 20, 2018

tfwww May 21, 2018

benmccann May 20, 2018 •

edited

Loading

tfwww May 21, 2018

benmccann May 20, 2018

tfwww May 21, 2018

benmccann May 20, 2018

tfwww May 21, 2018

benmccann May 21, 2018

benmccann May 21, 2018

tfwww May 21, 2018

benmccann May 21, 2018

tfwww May 21, 2018

benmccann May 23, 2018

tfwww May 24, 2018

benmccann Jun 15, 2018

benmccann Jun 15, 2018

benmccann Jun 15, 2018

benmccann commented Jul 7, 2018

benmccann commented Jul 30, 2018

benmccann commented Aug 3, 2018

andig commented Aug 8, 2018

benmccann Aug 8, 2018

benmccann commented Aug 8, 2018

benmccann commented Aug 15, 2018

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

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

Conversation

tfwww commented Feb 24, 2018 • edited Loading

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

benmccann May 20, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann commented Jul 7, 2018

benmccann commented Jul 30, 2018

benmccann commented Aug 3, 2018

andig commented Aug 8, 2018

Choose a reason for hiding this comment

benmccann commented Aug 8, 2018

benmccann commented Aug 15, 2018

tfwww commented Feb 24, 2018 •

edited

Loading

benmccann May 20, 2018 •

edited

Loading