From 50be2ce70aa1e6541c829eae8f404998a3c1504f Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Sun, 23 Jun 2019 21:02:10 -0700 Subject: [PATCH 1/5] AutoSkip in update --- src/core/core.scale.js | 99 ++++++++++++++++------------------ test/specs/core.scale.tests.js | 3 ++ 2 files changed, 49 insertions(+), 53 deletions(-) diff --git a/src/core/core.scale.js b/src/core/core.scale.js index 4dcd62680e4..f7aeeb58940 100644 --- a/src/core/core.scale.js +++ b/src/core/core.scale.js @@ -250,9 +250,18 @@ var Scale = Element.extend({ helpers.callback(this.options.beforeUpdate, [this]); }, + /** + * @param {number} maxWidth - the max width in pixels + * @param {number} maxHeight - the max height in pixels + * @param {object} margins - the space between the edge of the other scales and edge of the chart + * This space comes from two sources: + * - padding - space that's required to show the labels at the edges of the scale + * - thickness of scales or legends in another orientation + */ update: function(maxWidth, maxHeight, margins) { var me = this; - var i, ilen, labels, label, ticks, tick; + var tickOpts = me.options.ticks; + var i, ilen, labels, ticks; // Update Lifecycle - Probably don't want to ever extend or overwrite this function ;) me.beforeUpdate(); @@ -271,7 +280,6 @@ var Scale = Element.extend({ me._maxLabelLines = 0; me.longestLabelWidth = 0; me.longestTextCache = me.longestTextCache || {}; - me._ticksToDraw = null; me._gridLineItems = null; me._labelItems = null; @@ -296,10 +304,22 @@ var Scale = Element.extend({ // New implementations should return an array of objects but for BACKWARD COMPAT, // we still support no return (`this.ticks` internally set by calling this method). - ticks = me.buildTicks() || []; + ticks = me.buildTicks(); // Allow modification of ticks in callback. - ticks = me.afterBuildTicks(ticks) || ticks; + if (ticks) { + ticks = me.afterBuildTicks(ticks); + } else { + // Support old implementations (that modified `this.ticks` directly in buildTicks) + me.ticks = me.afterBuildTicks(me.ticks); + ticks = []; + for (i = 0, ilen = me.ticks.length; i < ilen; ++i) { + ticks.push({ + value: me.ticks[i], + major: false + }); + } + } me.beforeTickToLabelConversion(); @@ -310,22 +330,12 @@ var Scale = Element.extend({ me.afterTickToLabelConversion(); + // IMPORTANT: below this point, we consider that `this.ticks` will NEVER change! me.ticks = labels; // BACKWARD COMPATIBILITY - // IMPORTANT: from this point, we consider that `this.ticks` will NEVER change! - // BACKWARD COMPAT: synchronize `_ticks` with labels (so potentially `this.ticks`) for (i = 0, ilen = labels.length; i < ilen; ++i) { - label = labels[i]; - tick = ticks[i]; - if (!tick) { - ticks.push(tick = { - label: label, - major: false - }); - } else { - tick.label = label; - } + ticks[i].label = labels[i]; } me._ticks = ticks; @@ -344,9 +354,12 @@ var Scale = Element.extend({ me.beforeFit(); me.fit(); me.afterFit(); - // + // Auto-skip + me._ticksToDraw = tickOpts.display && tickOpts.autoSkip ? me._autoSkip(me.getTicks()) : ticks; + me.afterUpdate(); + // TODO: remove minSize as a public property and return value from all layout boxes. It is unused return me.minSize; }, @@ -425,13 +438,7 @@ var Scale = Element.extend({ buildTicks: helpers.noop, afterBuildTicks: function(ticks) { var me = this; - // ticks is empty for old axis implementations here - if (helpers.isArray(ticks) && ticks.length) { - return helpers.callback(me.options.afterBuildTicks, [me, ticks]); - } - // Support old implementations (that modified `this.ticks` directly in buildTicks) - me.ticks = helpers.callback(me.options.afterBuildTicks, [me, me.ticks]) || me.ticks; - return ticks; + return helpers.callback(me.options.afterBuildTicks, [me, ticks]) || ticks; }, beforeTickToLabelConversion: function() { @@ -508,6 +515,7 @@ var Scale = Element.extend({ height: 0 }; + var chart = me.chart; var opts = me.options; var tickOpts = opts.ticks; var scaleLabelOpts = opts.scaleLabel; @@ -593,8 +601,13 @@ var Scale = Element.extend({ me.handleMargins(); - me.width = minSize.width; - me.height = minSize.height; + if (isHorizontal) { + me.width = me._length = chart.width - me.margins.left - me.margins.right; + me.height = minSize.height; + } else { + me.width = minSize.width; + me.height = me._length = chart.height - me.margins.top - me.margins.bottom; + } }, /** @@ -603,12 +616,10 @@ var Scale = Element.extend({ */ handleMargins: function() { var me = this; - if (me.margins) { - me.margins.left = Math.max(me.paddingLeft, me.margins.left); - me.margins.top = Math.max(me.paddingTop, me.margins.top); - me.margins.right = Math.max(me.paddingRight, me.margins.right); - me.margins.bottom = Math.max(me.paddingBottom, me.margins.bottom); - } + me.margins.left = Math.max(me.paddingLeft, me.margins.left); + me.margins.top = Math.max(me.paddingTop, me.margins.top); + me.margins.right = Math.max(me.paddingRight, me.margins.right); + me.margins.bottom = Math.max(me.paddingBottom, me.margins.bottom); }, afterFit: function() { @@ -870,25 +881,6 @@ var Scale = Element.extend({ return false; }, - _getTicksToDraw: function() { - var me = this; - var optionTicks = me.options.ticks; - var ticks = me._ticksToDraw; - - if (ticks) { - return ticks; - } - - ticks = me.getTicks(); - - if (optionTicks.display && optionTicks.autoSkip) { - ticks = me._autoSkip(ticks); - } - - me._ticksToDraw = ticks; - return ticks; - }, - /** * @private */ @@ -900,8 +892,9 @@ var Scale = Element.extend({ var position = options.position; var offsetGridLines = gridLines.offsetGridLines; var isHorizontal = me.isHorizontal(); - var ticks = me._getTicksToDraw(); + var ticks = me._ticksToDraw; var ticksLength = ticks.length + (offsetGridLines ? 1 : 0); + var tl = getTickMarkLength(gridLines); var items = []; var axisWidth = gridLines.drawBorder ? valueAtIndexOrDefault(gridLines.lineWidth, 0, 0) : 0; @@ -1008,7 +1001,7 @@ var Scale = Element.extend({ var position = options.position; var isMirrored = optionTicks.mirror; var isHorizontal = me.isHorizontal(); - var ticks = me._getTicksToDraw(); + var ticks = me._ticksToDraw; var fonts = parseTickFontOptions(optionTicks); var tickPadding = optionTicks.padding; var tl = getTickMarkLength(options.gridLines); diff --git a/test/specs/core.scale.tests.js b/test/specs/core.scale.tests.js index a4f37460212..755bf37d8ba 100644 --- a/test/specs/core.scale.tests.js +++ b/test/specs/core.scale.tests.js @@ -495,6 +495,9 @@ describe('Core.scale', function() { it('should default to one layer for custom scales', function() { var customScale = Chart.Scale.extend({ draw: function() {}, + buildTicks: function() { + this.ticks = ['tick']; + }, convertTicksToLabels: function() { return ['tick']; } From ecc654af1432802fd58ddf4e1fb40e323db175b4 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Mon, 12 Aug 2019 00:48:13 -0700 Subject: [PATCH 2/5] Address review comments --- src/core/core.scale.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/core.scale.js b/src/core/core.scale.js index f7aeeb58940..f79b4f3c26c 100644 --- a/src/core/core.scale.js +++ b/src/core/core.scale.js @@ -330,9 +330,10 @@ var Scale = Element.extend({ me.afterTickToLabelConversion(); - // IMPORTANT: below this point, we consider that `this.ticks` will NEVER change! me.ticks = labels; // BACKWARD COMPATIBILITY + // IMPORTANT: below this point, we consider that `this.ticks` will NEVER change! + // BACKWARD COMPAT: synchronize `_ticks` with labels (so potentially `this.ticks`) for (i = 0, ilen = labels.length; i < ilen; ++i) { ticks[i].label = labels[i]; @@ -437,8 +438,7 @@ var Scale = Element.extend({ }, buildTicks: helpers.noop, afterBuildTicks: function(ticks) { - var me = this; - return helpers.callback(me.options.afterBuildTicks, [me, ticks]) || ticks; + return helpers.callback(this.options.afterBuildTicks, [this, ticks]) || ticks; }, beforeTickToLabelConversion: function() { From 1dec31dc286649e728d5a0390d237b9df209ddc4 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Mon, 12 Aug 2019 23:56:05 -0700 Subject: [PATCH 3/5] Add v3 to TODO --- src/core/core.scale.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/core.scale.js b/src/core/core.scale.js index f79b4f3c26c..1430cd62bd7 100644 --- a/src/core/core.scale.js +++ b/src/core/core.scale.js @@ -360,7 +360,7 @@ var Scale = Element.extend({ me.afterUpdate(); - // TODO: remove minSize as a public property and return value from all layout boxes. It is unused + // TODO(v3): remove minSize as a public property and return value from all layout boxes. It is unused return me.minSize; }, From e5c81501c395b71f53f5e1f449ebdf0fe96a3309 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Fri, 16 Aug 2019 00:28:16 -0700 Subject: [PATCH 4/5] Address review comments --- src/core/core.scale.js | 9 +++++---- test/specs/core.scale.tests.js | 3 --- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/core/core.scale.js b/src/core/core.scale.js index 1430cd62bd7..634c942ca40 100644 --- a/src/core/core.scale.js +++ b/src/core/core.scale.js @@ -308,10 +308,10 @@ var Scale = Element.extend({ // Allow modification of ticks in callback. if (ticks) { - ticks = me.afterBuildTicks(ticks); + ticks = me.afterBuildTicks(ticks || []); } else { // Support old implementations (that modified `this.ticks` directly in buildTicks) - me.ticks = me.afterBuildTicks(me.ticks); + me.ticks = me.afterBuildTicks(me.ticks || []); ticks = []; for (i = 0, ilen = me.ticks.length; i < ilen; ++i) { ticks.push({ @@ -335,7 +335,7 @@ var Scale = Element.extend({ // IMPORTANT: below this point, we consider that `this.ticks` will NEVER change! // BACKWARD COMPAT: synchronize `_ticks` with labels (so potentially `this.ticks`) - for (i = 0, ilen = labels.length; i < ilen; ++i) { + for (i = 0, ilen = ticks.length; i < ilen; ++i) { ticks[i].label = labels[i]; } @@ -356,11 +356,12 @@ var Scale = Element.extend({ me.fit(); me.afterFit(); // Auto-skip - me._ticksToDraw = tickOpts.display && tickOpts.autoSkip ? me._autoSkip(me.getTicks()) : ticks; + me._ticksToDraw = tickOpts.display && tickOpts.autoSkip ? me._autoSkip(me._ticks) : me._ticks; me.afterUpdate(); // TODO(v3): remove minSize as a public property and return value from all layout boxes. It is unused + // make maxWidth and maxHeight private return me.minSize; }, diff --git a/test/specs/core.scale.tests.js b/test/specs/core.scale.tests.js index 755bf37d8ba..a4f37460212 100644 --- a/test/specs/core.scale.tests.js +++ b/test/specs/core.scale.tests.js @@ -495,9 +495,6 @@ describe('Core.scale', function() { it('should default to one layer for custom scales', function() { var customScale = Chart.Scale.extend({ draw: function() {}, - buildTicks: function() { - this.ticks = ['tick']; - }, convertTicksToLabels: function() { return ['tick']; } From 557b9ca3ab58c5202e67fad0c6db192b97556f80 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Thu, 22 Aug 2019 10:58:11 -0700 Subject: [PATCH 5/5] Remove unrelated code cleanup --- src/core/core.scale.js | 50 +++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/src/core/core.scale.js b/src/core/core.scale.js index 634c942ca40..97a62f5695e 100644 --- a/src/core/core.scale.js +++ b/src/core/core.scale.js @@ -261,7 +261,7 @@ var Scale = Element.extend({ update: function(maxWidth, maxHeight, margins) { var me = this; var tickOpts = me.options.ticks; - var i, ilen, labels, ticks; + var i, ilen, labels, label, ticks, tick; // Update Lifecycle - Probably don't want to ever extend or overwrite this function ;) me.beforeUpdate(); @@ -304,22 +304,10 @@ var Scale = Element.extend({ // New implementations should return an array of objects but for BACKWARD COMPAT, // we still support no return (`this.ticks` internally set by calling this method). - ticks = me.buildTicks(); + ticks = me.buildTicks() || []; // Allow modification of ticks in callback. - if (ticks) { - ticks = me.afterBuildTicks(ticks || []); - } else { - // Support old implementations (that modified `this.ticks` directly in buildTicks) - me.ticks = me.afterBuildTicks(me.ticks || []); - ticks = []; - for (i = 0, ilen = me.ticks.length; i < ilen; ++i) { - ticks.push({ - value: me.ticks[i], - major: false - }); - } - } + ticks = me.afterBuildTicks(ticks) || ticks; me.beforeTickToLabelConversion(); @@ -335,8 +323,17 @@ var Scale = Element.extend({ // IMPORTANT: below this point, we consider that `this.ticks` will NEVER change! // BACKWARD COMPAT: synchronize `_ticks` with labels (so potentially `this.ticks`) - for (i = 0, ilen = ticks.length; i < ilen; ++i) { - ticks[i].label = labels[i]; + for (i = 0, ilen = labels.length; i < ilen; ++i) { + label = labels[i]; + tick = ticks[i]; + if (!tick) { + ticks.push(tick = { + label: label, + major: false + }); + } else { + tick.label = label; + } } me._ticks = ticks; @@ -439,7 +436,14 @@ var Scale = Element.extend({ }, buildTicks: helpers.noop, afterBuildTicks: function(ticks) { - return helpers.callback(this.options.afterBuildTicks, [this, ticks]) || ticks; + var me = this; + // ticks is empty for old axis implementations here + if (helpers.isArray(ticks) && ticks.length) { + return helpers.callback(me.options.afterBuildTicks, [me, ticks]); + } + // Support old implementations (that modified `this.ticks` directly in buildTicks) + me.ticks = helpers.callback(me.options.afterBuildTicks, [me, me.ticks]) || me.ticks; + return ticks; }, beforeTickToLabelConversion: function() { @@ -617,10 +621,12 @@ var Scale = Element.extend({ */ handleMargins: function() { var me = this; - me.margins.left = Math.max(me.paddingLeft, me.margins.left); - me.margins.top = Math.max(me.paddingTop, me.margins.top); - me.margins.right = Math.max(me.paddingRight, me.margins.right); - me.margins.bottom = Math.max(me.paddingBottom, me.margins.bottom); + if (me.margins) { + me.margins.left = Math.max(me.paddingLeft, me.margins.left); + me.margins.top = Math.max(me.paddingTop, me.margins.top); + me.margins.right = Math.max(me.paddingRight, me.margins.right); + me.margins.bottom = Math.max(me.paddingBottom, me.margins.bottom); + } }, afterFit: function() {