From 3d6e5d18b2364f24cd0fb1cd4076b37365099663 Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Sun, 16 Dec 2018 21:12:22 +0200 Subject: [PATCH 1/9] Return correct label for value type axis --- src/scales/scale.category.js | 14 ++++++-- test/specs/scale.category.tests.js | 57 ++++++++++++++++-------------- 2 files changed, 42 insertions(+), 29 deletions(-) diff --git a/src/scales/scale.category.js b/src/scales/scale.category.js index 3a4d20645b3..3a701d345d1 100644 --- a/src/scales/scale.category.js +++ b/src/scales/scale.category.js @@ -49,10 +49,20 @@ module.exports = Scale.extend({ getLabelForIndex: function(index, datasetIndex) { var me = this; - var data = me.chart.data; + var chart = me.chart; + var data = chart.data; var isHorizontal = me.isHorizontal(); - if (data.yLabels && !isHorizontal) { + var ds = chart.getDatasetMeta(datasetIndex).controller; + + // Do we have a getValuesScaleId function? (eg. bar chart) + var isValueScale = ds.getValueScaleId + ? ds.getValueScaleId() === me.id + : !isHorizontal; + + // FIXME For non-bar charts we assume vertical axis is a value scale + // and thus should return getRightValue + if (isValueScale) { return me.getRightValue(data.datasets[datasetIndex].data[index]); } return me.ticks[index - me.minIndex]; diff --git a/test/specs/scale.category.tests.js b/test/specs/scale.category.tests.js index 7bc6a8a57ad..d6529324ec3 100644 --- a/test/specs/scale.category.tests.js +++ b/test/specs/scale.category.tests.js @@ -156,35 +156,38 @@ describe('Category scale tests', function() { expect(scale.ticks).toEqual(labels); }); - it ('should get the correct label for the index', function() { - var scaleID = 'myScale'; - - var mockData = { - datasets: [{ - yAxisID: scaleID, - data: [10, 5, 0, 25, 78] - }], - labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'] - }; - - var config = Chart.helpers.clone(Chart.scaleService.getScaleDefaults('category')); - var Constructor = Chart.scaleService.getScaleConstructor('category'); - var scale = new Constructor({ - ctx: {}, - options: config, - chart: { - data: mockData + it('should get the correct label for the index', function() { + var chart = window.acquireChart({ + type: 'line', + data: { + datasets: [{ + xAxisID: 'xScale0', + yAxisID: 'yScale0', + data: [10, 5, 0, 25, 78] + }], + labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'] }, - id: scaleID + options: { + scales: { + xAxes: [{ + id: 'xScale0', + type: 'category', + position: 'bottom' + }], + yAxes: [{ + id: 'yScale0', + type: 'linear' + }] + } + } }); - scale.determineDataLimits(); - scale.buildTicks(); + var scale = chart.scales.xScale0; - expect(scale.getLabelForIndex(1)).toBe('tick2'); + expect(scale.getLabelForIndex(1, 0)).toBe('tick2'); }); - it ('Should get the correct pixel for a value when horizontal', function() { + it('Should get the correct pixel for a value when horizontal', function() { var chart = window.acquireChart({ type: 'line', data: { @@ -227,7 +230,7 @@ describe('Category scale tests', function() { expect(xScale.getValueForPixel(417)).toBe(4); }); - it ('Should get the correct pixel for a value when there are repeated labels', function() { + it('Should get the correct pixel for a value when there are repeated labels', function() { var chart = window.acquireChart({ type: 'line', data: { @@ -258,7 +261,7 @@ describe('Category scale tests', function() { expect(xScale.getPixelForValue('tick_1', 1, 0)).toBeCloseToPixel(143); }); - it ('Should get the correct pixel for a value when horizontal and zoomed', function() { + it('Should get the correct pixel for a value when horizontal and zoomed', function() { var chart = window.acquireChart({ type: 'line', data: { @@ -299,7 +302,7 @@ describe('Category scale tests', function() { expect(xScale.getPixelForValue(0, 3, 0)).toBeCloseToPixel(429); }); - it ('should get the correct pixel for a value when vertical', function() { + it('should get the correct pixel for a value when vertical', function() { var chart = window.acquireChart({ type: 'line', data: { @@ -344,7 +347,7 @@ describe('Category scale tests', function() { expect(yScale.getValueForPixel(437)).toBe(4); }); - it ('should get the correct pixel for a value when vertical and zoomed', function() { + it('should get the correct pixel for a value when vertical and zoomed', function() { var chart = window.acquireChart({ type: 'line', data: { From a170babdbe775a19a05285027be5c328ec31014a Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Thu, 10 Jan 2019 20:18:24 +0200 Subject: [PATCH 2/9] update comments --- src/scales/scale.category.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/scales/scale.category.js b/src/scales/scale.category.js index 3a701d345d1..c1a2776d1a2 100644 --- a/src/scales/scale.category.js +++ b/src/scales/scale.category.js @@ -55,13 +55,14 @@ module.exports = Scale.extend({ var ds = chart.getDatasetMeta(datasetIndex).controller; - // Do we have a getValuesScaleId function? (eg. bar chart) + // For scales supporting getValueScaleId, we can be sure if this is a value scale. + // For others, vertical scale is assumed to be value. + // Note: This assumptiom works correctly for all charts with {x,y} data, + // because getRightValue returns correct thing regardless if its a label or value. var isValueScale = ds.getValueScaleId ? ds.getValueScaleId() === me.id : !isHorizontal; - // FIXME For non-bar charts we assume vertical axis is a value scale - // and thus should return getRightValue if (isValueScale) { return me.getRightValue(data.datasets[datasetIndex].data[index]); } From e28606310d40b5aef44d07040d198cae5f2f6892 Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Thu, 10 Jan 2019 20:43:01 +0200 Subject: [PATCH 3/9] fix typo --- src/scales/scale.category.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scales/scale.category.js b/src/scales/scale.category.js index c1a2776d1a2..331ee21a946 100644 --- a/src/scales/scale.category.js +++ b/src/scales/scale.category.js @@ -57,7 +57,7 @@ module.exports = Scale.extend({ // For scales supporting getValueScaleId, we can be sure if this is a value scale. // For others, vertical scale is assumed to be value. - // Note: This assumptiom works correctly for all charts with {x,y} data, + // Note: This assumption works correctly for all charts with {x,y} data, // because getRightValue returns correct thing regardless if its a label or value. var isValueScale = ds.getValueScaleId ? ds.getValueScaleId() === me.id From 969639fcc1c827291ef71ddbe8ad1d2c75ad88c3 Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Thu, 10 Jan 2019 23:05:08 +0200 Subject: [PATCH 4/9] modify comment --- src/scales/scale.category.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/scales/scale.category.js b/src/scales/scale.category.js index 331ee21a946..a10bdb3592f 100644 --- a/src/scales/scale.category.js +++ b/src/scales/scale.category.js @@ -55,10 +55,10 @@ module.exports = Scale.extend({ var ds = chart.getDatasetMeta(datasetIndex).controller; - // For scales supporting getValueScaleId, we can be sure if this is a value scale. + // For controllers supporting getValueScaleId, we can be sure if this is a value scale. // For others, vertical scale is assumed to be value. // Note: This assumption works correctly for all charts with {x,y} data, - // because getRightValue returns correct thing regardless if its a label or value. + // because getRightValue returns correct thing regardless if it's a index or value. var isValueScale = ds.getValueScaleId ? ds.getValueScaleId() === me.id : !isHorizontal; From 08b101b5bc830157f412908da1f1c630499dae71 Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Sun, 13 Jan 2019 19:28:43 +0200 Subject: [PATCH 5/9] Make sure assumption is right. --- src/scales/scale.category.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/scales/scale.category.js b/src/scales/scale.category.js index a10bdb3592f..d1c63d6433d 100644 --- a/src/scales/scale.category.js +++ b/src/scales/scale.category.js @@ -1,5 +1,6 @@ 'use strict'; +var helpers = require('../helpers/index'); var Scale = require('../core/core.scale'); var defaultConfig = { @@ -52,16 +53,14 @@ module.exports = Scale.extend({ var chart = me.chart; var data = chart.data; var isHorizontal = me.isHorizontal(); - - var ds = chart.getDatasetMeta(datasetIndex).controller; + var controller = chart.getDatasetMeta(datasetIndex).controller; // For controllers supporting getValueScaleId, we can be sure if this is a value scale. - // For others, vertical scale is assumed to be value. - // Note: This assumption works correctly for all charts with {x,y} data, - // because getRightValue returns correct thing regardless if it's a index or value. - var isValueScale = ds.getValueScaleId - ? ds.getValueScaleId() === me.id - : !isHorizontal; + // For object data ({x,y} etc.) we can rely on getRightValue to return correct thing. + // For others, check if data is a label and use that instead (so this is an value index, but values are labels) + var isValueScale = controller.getValueScaleId + ? controller.getValueScaleId() === me.id + : helpers.isObject(data[0]) || (!isHorizontal && me.getLabels().indexOf(data.datasets[datasetIndex].data[index]) !== -1); if (isValueScale) { return me.getRightValue(data.datasets[datasetIndex].data[index]); From 89852d84f626c0ff61e17b877e8ffd4389b3dd5c Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Sun, 13 Jan 2019 21:21:52 +0200 Subject: [PATCH 6/9] Alias data, cache value with index check Fix prepositions --- src/scales/scale.category.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/scales/scale.category.js b/src/scales/scale.category.js index d1c63d6433d..8886ff5613c 100644 --- a/src/scales/scale.category.js +++ b/src/scales/scale.category.js @@ -51,20 +51,23 @@ module.exports = Scale.extend({ getLabelForIndex: function(index, datasetIndex) { var me = this; var chart = me.chart; - var data = chart.data; + var data = chart.data.datasets[datasetIndex].data; + var value = data && index < data.length ? data[index] : false; var isHorizontal = me.isHorizontal(); var controller = chart.getDatasetMeta(datasetIndex).controller; // For controllers supporting getValueScaleId, we can be sure if this is a value scale. // For object data ({x,y} etc.) we can rely on getRightValue to return correct thing. - // For others, check if data is a label and use that instead (so this is an value index, but values are labels) - var isValueScale = controller.getValueScaleId + // For others, check if value is a label and use that instead (so `index` is an index of a value, but values are labels) + var useGetRightValue = controller.getValueScaleId ? controller.getValueScaleId() === me.id - : helpers.isObject(data[0]) || (!isHorizontal && me.getLabels().indexOf(data.datasets[datasetIndex].data[index]) !== -1); + : value && (helpers.isObject(value) || (!isHorizontal && me.getLabels().indexOf(value) !== -1)); - if (isValueScale) { - return me.getRightValue(data.datasets[datasetIndex].data[index]); + if (useGetRightValue) { + return me.getRightValue(value); } + + // So `index` is an index of a label (aka tick) return me.ticks[index - me.minIndex]; }, From dc8a68659b296ced1747705988a2a9e9bc074c71 Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Sun, 13 Jan 2019 21:52:52 +0200 Subject: [PATCH 7/9] make isHorizontal matter for object data too. update comments --- src/scales/scale.category.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/scales/scale.category.js b/src/scales/scale.category.js index 8886ff5613c..8f729b35518 100644 --- a/src/scales/scale.category.js +++ b/src/scales/scale.category.js @@ -57,11 +57,13 @@ module.exports = Scale.extend({ var controller = chart.getDatasetMeta(datasetIndex).controller; // For controllers supporting getValueScaleId, we can be sure if this is a value scale. - // For object data ({x,y} etc.) we can rely on getRightValue to return correct thing. - // For others, check if value is a label and use that instead (so `index` is an index of a value, but values are labels) + // For vertical scale, assume chart orientation is horizontal and check + // 1. Value is object ({x,y} etc.) we can rely on getRightValue to return correct thing. + // 2. Value is a label, use that instead (so `index` is an index of a value, but values are labels) + // * getRightValue call is not needed in this case, but its ok. var useGetRightValue = controller.getValueScaleId ? controller.getValueScaleId() === me.id - : value && (helpers.isObject(value) || (!isHorizontal && me.getLabels().indexOf(value) !== -1)); + : !isHorizontal && value && (helpers.isObject(value) || (me.getLabels().indexOf(value) !== -1)); if (useGetRightValue) { return me.getRightValue(value); From 4abdc599807bb8b90b2ef194f13f7e087c0c6812 Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Tue, 15 Jan 2019 17:13:02 +0200 Subject: [PATCH 8/9] move getValueScale + friends up to DatasetController --- src/controllers/controller.bar.js | 28 ---------------------------- src/core/core.datasetController.js | 28 ++++++++++++++++++++++++++++ src/scales/scale.category.js | 21 +++------------------ 3 files changed, 31 insertions(+), 46 deletions(-) diff --git a/src/controllers/controller.bar.js b/src/controllers/controller.bar.js index cd611657275..ed6b6c6afb7 100644 --- a/src/controllers/controller.bar.js +++ b/src/controllers/controller.bar.js @@ -188,34 +188,6 @@ module.exports = DatasetController.extend({ model.width = horizontal ? undefined : ipixels.size; }, - /** - * @private - */ - getValueScaleId: function() { - return this.getMeta().yAxisID; - }, - - /** - * @private - */ - getIndexScaleId: function() { - return this.getMeta().xAxisID; - }, - - /** - * @private - */ - getValueScale: function() { - return this.getScaleForId(this.getValueScaleId()); - }, - - /** - * @private - */ - getIndexScale: function() { - return this.getScaleForId(this.getIndexScaleId()); - }, - /** * Returns the stacks based on groups and bar visibility. * @param {Number} [last] - The dataset index diff --git a/src/core/core.datasetController.js b/src/core/core.datasetController.js index ab7ca1282c8..383e4fad9a6 100644 --- a/src/core/core.datasetController.js +++ b/src/core/core.datasetController.js @@ -131,6 +131,34 @@ helpers.extend(DatasetController.prototype, { return this.chart.scales[scaleID]; }, + /** + * @private + */ + getValueScaleId: function() { + return this.getMeta().yAxisID; + }, + + /** + * @private + */ + getIndexScaleId: function() { + return this.getMeta().xAxisID; + }, + + /** + * @private + */ + getValueScale: function() { + return this.getScaleForId(this.getValueScaleId()); + }, + + /** + * @private + */ + getIndexScale: function() { + return this.getScaleForId(this.getIndexScaleId()); + }, + reset: function() { this.update(true); }, diff --git a/src/scales/scale.category.js b/src/scales/scale.category.js index 8f729b35518..d67694e06dd 100644 --- a/src/scales/scale.category.js +++ b/src/scales/scale.category.js @@ -1,6 +1,5 @@ 'use strict'; -var helpers = require('../helpers/index'); var Scale = require('../core/core.scale'); var defaultConfig = { @@ -51,25 +50,11 @@ module.exports = Scale.extend({ getLabelForIndex: function(index, datasetIndex) { var me = this; var chart = me.chart; - var data = chart.data.datasets[datasetIndex].data; - var value = data && index < data.length ? data[index] : false; - var isHorizontal = me.isHorizontal(); - var controller = chart.getDatasetMeta(datasetIndex).controller; - - // For controllers supporting getValueScaleId, we can be sure if this is a value scale. - // For vertical scale, assume chart orientation is horizontal and check - // 1. Value is object ({x,y} etc.) we can rely on getRightValue to return correct thing. - // 2. Value is a label, use that instead (so `index` is an index of a value, but values are labels) - // * getRightValue call is not needed in this case, but its ok. - var useGetRightValue = controller.getValueScaleId - ? controller.getValueScaleId() === me.id - : !isHorizontal && value && (helpers.isObject(value) || (me.getLabels().indexOf(value) !== -1)); - - if (useGetRightValue) { - return me.getRightValue(value); + + if (chart.getDatasetMeta(datasetIndex).controller.getValueScaleId() === me.id) { + return me.getRightValue(chart.data.datasets[datasetIndex].data[index]); } - // So `index` is an index of a label (aka tick) return me.ticks[index - me.minIndex]; }, From bacc5e943ade0090a6c04483fb774b3c5214ca2a Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Wed, 16 Jan 2019 14:38:56 +0200 Subject: [PATCH 9/9] prefix moved private methods with underscore --- src/controllers/controller.bar.js | 12 ++++++------ src/controllers/controller.horizontalBar.js | 4 ++-- src/core/core.datasetController.js | 12 ++++++------ src/scales/scale.category.js | 2 +- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/controllers/controller.bar.js b/src/controllers/controller.bar.js index ed6b6c6afb7..aef53b68afe 100644 --- a/src/controllers/controller.bar.js +++ b/src/controllers/controller.bar.js @@ -173,7 +173,7 @@ module.exports = DatasetController.extend({ _updateElementGeometry: function(rectangle, index, reset) { var me = this; var model = rectangle._model; - var vscale = me.getValueScale(); + var vscale = me._getValueScale(); var base = vscale.getBasePixel(); var horizontal = vscale.isHorizontal(); var ruler = me._ruler || me.getRuler(); @@ -197,7 +197,7 @@ module.exports = DatasetController.extend({ _getStacks: function(last) { var me = this; var chart = me.chart; - var scale = me.getIndexScale(); + var scale = me._getIndexScale(); var stacked = scale.options.stacked; var ilen = last === undefined ? chart.data.datasets.length : last + 1; var stacks = []; @@ -247,7 +247,7 @@ module.exports = DatasetController.extend({ */ getRuler: function() { var me = this; - var scale = me.getIndexScale(); + var scale = me._getIndexScale(); var stackCount = me.getStackCount(); var datasetIndex = me.index; var isHorizontal = scale.isHorizontal(); @@ -282,7 +282,7 @@ module.exports = DatasetController.extend({ var me = this; var chart = me.chart; var meta = me.getMeta(); - var scale = me.getValueScale(); + var scale = me._getValueScale(); var isHorizontal = scale.isHorizontal(); var datasets = chart.data.datasets; var value = +scale.getRightValue(datasets[datasetIndex].data[index]); @@ -298,7 +298,7 @@ module.exports = DatasetController.extend({ if (imeta.bar && imeta.stack === stack && - imeta.controller.getValueScaleId() === scale.id && + imeta.controller._getValueScaleId() === scale.id && chart.isDatasetVisible(i)) { ivalue = +scale.getRightValue(datasets[i].data[index]); @@ -357,7 +357,7 @@ module.exports = DatasetController.extend({ draw: function() { var me = this; var chart = me.chart; - var scale = me.getValueScale(); + var scale = me._getValueScale(); var rects = me.getMeta().data; var dataset = me.getDataset(); var ilen = rects.length; diff --git a/src/controllers/controller.horizontalBar.js b/src/controllers/controller.horizontalBar.js index b761c344403..85d31ba515e 100644 --- a/src/controllers/controller.horizontalBar.js +++ b/src/controllers/controller.horizontalBar.js @@ -65,14 +65,14 @@ module.exports = BarController.extend({ /** * @private */ - getValueScaleId: function() { + _getValueScaleId: function() { return this.getMeta().xAxisID; }, /** * @private */ - getIndexScaleId: function() { + _getIndexScaleId: function() { return this.getMeta().yAxisID; } }); diff --git a/src/core/core.datasetController.js b/src/core/core.datasetController.js index 383e4fad9a6..303549849ae 100644 --- a/src/core/core.datasetController.js +++ b/src/core/core.datasetController.js @@ -134,29 +134,29 @@ helpers.extend(DatasetController.prototype, { /** * @private */ - getValueScaleId: function() { + _getValueScaleId: function() { return this.getMeta().yAxisID; }, /** * @private */ - getIndexScaleId: function() { + _getIndexScaleId: function() { return this.getMeta().xAxisID; }, /** * @private */ - getValueScale: function() { - return this.getScaleForId(this.getValueScaleId()); + _getValueScale: function() { + return this.getScaleForId(this._getValueScaleId()); }, /** * @private */ - getIndexScale: function() { - return this.getScaleForId(this.getIndexScaleId()); + _getIndexScale: function() { + return this.getScaleForId(this._getIndexScaleId()); }, reset: function() { diff --git a/src/scales/scale.category.js b/src/scales/scale.category.js index d67694e06dd..d837a7365da 100644 --- a/src/scales/scale.category.js +++ b/src/scales/scale.category.js @@ -51,7 +51,7 @@ module.exports = Scale.extend({ var me = this; var chart = me.chart; - if (chart.getDatasetMeta(datasetIndex).controller.getValueScaleId() === me.id) { + if (chart.getDatasetMeta(datasetIndex).controller._getValueScaleId() === me.id) { return me.getRightValue(chart.data.datasets[datasetIndex].data[index]); }