From b97df7c95f0d59d46647c6051d26ee58ae78b459 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Sun, 27 Jan 2019 20:19:33 -0800 Subject: [PATCH 1/6] Time scale performance improvement --- src/scales/scale.time.js | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/scales/scale.time.js b/src/scales/scale.time.js index 8dfa4e36b00..d2efe6e0cc4 100644 --- a/src/scales/scale.time.js +++ b/src/scales/scale.time.js @@ -408,21 +408,12 @@ function ticksFromTimestamps(values, majorUnit) { /** * Return the time format for the label with the most parts (milliseconds, second, etc.) */ -function determineLabelFormat(timestamps) { +function determineLabelFormat(timestamp) { var presets = adapter.presets(); - var ilen = timestamps.length; - var i, ts, hasTime; - - for (i = 0; i < ilen; i++) { - ts = timestamps[i]; - if (ts % INTERVALS.second.size !== 0) { - return presets.full; - } - if (!hasTime && adapter.startOf(ts, 'day') !== ts) { - hasTime = true; - } + if (timestamp % INTERVALS.second.size !== 0) { + return presets.full; } - if (hasTime) { + if (adapter.startOf(timestamp, 'day') !== timestamp) { return presets.time; } return presets.date; @@ -637,7 +628,6 @@ module.exports = Scale.extend({ me._majorUnit = determineMajorUnit(me._unit); me._table = buildLookupTable(me._timestamps.data, min, max, options.distribution); me._offsets = computeOffsets(me._table, ticks, min, max, options); - me._labelFormat = determineLabelFormat(me._timestamps.data); if (options.ticks.reverse) { ticks.reverse(); @@ -652,6 +642,7 @@ module.exports = Scale.extend({ var timeOpts = me.options.time; var label = data.labels && index < data.labels.length ? data.labels[index] : ''; var value = data.datasets[datasetIndex].data[index]; + var ts; if (helpers.isObject(value)) { label = me.getRightValue(value); @@ -663,7 +654,8 @@ module.exports = Scale.extend({ return label; } - return adapter.format(toTimestamp(label, timeOpts), me._labelFormat); + ts = toTimestamp(label, timeOpts); + return adapter.format(ts, determineLabelFormat(ts)); }, /** From c2a4c17ab9bceaed7453eeeefc3c24f5244ef548 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Sun, 3 Feb 2019 09:59:25 -0800 Subject: [PATCH 2/6] Use tick format for tooltips as well --- src/scales/scale.time.js | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/src/scales/scale.time.js b/src/scales/scale.time.js index d2efe6e0cc4..5f57915e28d 100644 --- a/src/scales/scale.time.js +++ b/src/scales/scale.time.js @@ -405,20 +405,6 @@ function ticksFromTimestamps(values, majorUnit) { return ticks; } -/** - * Return the time format for the label with the most parts (milliseconds, second, etc.) - */ -function determineLabelFormat(timestamp) { - var presets = adapter.presets(); - if (timestamp % INTERVALS.second.size !== 0) { - return presets.full; - } - if (adapter.startOf(timestamp, 'day') !== timestamp) { - return presets.time; - } - return presets.date; -} - var defaultConfig = { position: 'bottom', @@ -642,7 +628,6 @@ module.exports = Scale.extend({ var timeOpts = me.options.time; var label = data.labels && index < data.labels.length ? data.labels[index] : ''; var value = data.datasets[datasetIndex].data[index]; - var ts; if (helpers.isObject(value)) { label = me.getRightValue(value); @@ -653,9 +638,7 @@ module.exports = Scale.extend({ if (typeof label === 'string') { return label; } - - ts = toTimestamp(label, timeOpts); - return adapter.format(ts, determineLabelFormat(ts)); + return adapter.format(toTimestamp(label, timeOpts), timeOpts.displayFormats[me._unit]); }, /** From e5862d31f123dbff16532db1044ab1421d4b9e4a Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Mon, 4 Feb 2019 07:29:39 -0800 Subject: [PATCH 3/6] Update tests --- test/specs/scale.time.tests.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/specs/scale.time.tests.js b/test/specs/scale.time.tests.js index c5be682d95d..3218de65e11 100755 --- a/test/specs/scale.time.tests.js +++ b/test/specs/scale.time.tests.js @@ -624,7 +624,7 @@ describe('Time scale tests', function() { var xScale = chart.scales.xScale0; var label = xScale.getLabelForIndex(0, 0); - expect(label).toEqual('Jan 8, 2018 5:14:23.234 am'); + expect(label).toEqual('5AM'); }); it('should get the correct label for a timestamp with time', function() { @@ -652,7 +652,7 @@ describe('Time scale tests', function() { var xScale = chart.scales.xScale0; var label = xScale.getLabelForIndex(0, 0); - expect(label).toEqual('Jan 8, 2018 5:14:23 am'); + expect(label).toEqual('5AM'); }); it('should get the correct label for a timestamp representing a date', function() { @@ -680,7 +680,7 @@ describe('Time scale tests', function() { var xScale = chart.scales.xScale0; var label = xScale.getLabelForIndex(0, 0); - expect(label).toEqual('Jan 8, 2018'); + expect(label).toEqual('12AM'); }); it('should get the correct pixel for only one data in the dataset', function() { From bbbd98507751ae5f35288a11d3554b3e406ca4a0 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Wed, 13 Feb 2019 16:41:41 -0800 Subject: [PATCH 4/6] Address review feedback --- src/adapters/adapter.moment.js | 11 +----- src/core/core.adapters.js | 9 ----- src/scales/scale.time.js | 2 +- test/specs/scale.time.tests.js | 62 +++------------------------------- 4 files changed, 6 insertions(+), 78 deletions(-) diff --git a/src/adapters/adapter.moment.js b/src/adapters/adapter.moment.js index 04c03155924..2ae611dfe2a 100644 --- a/src/adapters/adapter.moment.js +++ b/src/adapters/adapter.moment.js @@ -7,6 +7,7 @@ var adapter = require('../core/core.adapters')._date; var helpers = require('../helpers/helpers.core'); var FORMATS = { + datetime: 'MMM D, YYYY, h:mm:ss a', millisecond: 'h:mm:ss.SSS a', second: 'h:mm:ss a', minute: 'h:mm a', @@ -18,12 +19,6 @@ var FORMATS = { year: 'YYYY' }; -var PRESETS = { - full: 'MMM D, YYYY h:mm:ss.SSS a', - time: 'MMM D, YYYY h:mm:ss a', - date: 'MMM D, YYYY' -}; - helpers.merge(adapter, moment ? { _id: 'moment', // DEBUG ONLY @@ -31,10 +26,6 @@ helpers.merge(adapter, moment ? { return FORMATS; }, - presets: function() { - return PRESETS; - }, - parse: function(value, format) { if (typeof value === 'string' && typeof format === 'string') { value = moment(value, format); diff --git a/src/core/core.adapters.js b/src/core/core.adapters.js index 5aa78e0765b..8f61fbcc235 100644 --- a/src/core/core.adapters.js +++ b/src/core/core.adapters.js @@ -35,15 +35,6 @@ module.exports._date = { */ formats: abstract, - /** - * Returns a map of date/time formats for the following presets: - * 'full': date + time + millisecond - * 'time': date + time - * 'date': date - * @returns {{string: string}} - */ - presets: abstract, - /** * Parses the given `value` and return the associated timestamp. * @param {any} value - the value to parse (usually comes from the data) diff --git a/src/scales/scale.time.js b/src/scales/scale.time.js index 5f57915e28d..1f2390850bd 100644 --- a/src/scales/scale.time.js +++ b/src/scales/scale.time.js @@ -638,7 +638,7 @@ module.exports = Scale.extend({ if (typeof label === 'string') { return label; } - return adapter.format(toTimestamp(label, timeOpts), timeOpts.displayFormats[me._unit]); + return adapter.format(toTimestamp(label, timeOpts), timeOpts.displayFormats.datetime); }, /** diff --git a/test/specs/scale.time.tests.js b/test/specs/scale.time.tests.js index 3218de65e11..8c516297554 100755 --- a/test/specs/scale.time.tests.js +++ b/test/specs/scale.time.tests.js @@ -599,7 +599,7 @@ describe('Time scale tests', function() { expect(xScale.getLabelForIndex(0, 0)).toBe('2015-01-01T20:00:00'); }); - it('should get the correct label for a timestamp with milliseconds', function() { + it('should get the correct label for a timestamp', function() { var chart = window.acquireChart({ type: 'line', data: { @@ -624,63 +624,7 @@ describe('Time scale tests', function() { var xScale = chart.scales.xScale0; var label = xScale.getLabelForIndex(0, 0); - expect(label).toEqual('5AM'); - }); - - it('should get the correct label for a timestamp with time', function() { - var chart = window.acquireChart({ - type: 'line', - data: { - datasets: [{ - xAxisID: 'xScale0', - data: [ - {t: +new Date('2018-01-08 05:14:23'), y: 10}, - {t: +new Date('2018-01-09 06:17:43'), y: 3} - ] - }], - }, - options: { - scales: { - xAxes: [{ - id: 'xScale0', - type: 'time', - position: 'bottom' - }], - } - } - }); - - var xScale = chart.scales.xScale0; - var label = xScale.getLabelForIndex(0, 0); - expect(label).toEqual('5AM'); - }); - - it('should get the correct label for a timestamp representing a date', function() { - var chart = window.acquireChart({ - type: 'line', - data: { - datasets: [{ - xAxisID: 'xScale0', - data: [ - {t: +new Date('2018-01-08 00:00:00'), y: 10}, - {t: +new Date('2018-01-09 00:00:00'), y: 3} - ] - }], - }, - options: { - scales: { - xAxes: [{ - id: 'xScale0', - type: 'time', - position: 'bottom' - }], - } - } - }); - - var xScale = chart.scales.xScale0; - var label = xScale.getLabelForIndex(0, 0); - expect(label).toEqual('12AM'); + expect(label).toEqual('Jan 8, 2018, 5:14:23 am'); }); it('should get the correct pixel for only one data in the dataset', function() { @@ -1532,6 +1476,7 @@ describe('Time scale tests', function() { // NOTE: built-in adapter uses moment var expected = { + datetime: 'MMM D, YYYY, h:mm:ss a', millisecond: 'h:mm:ss.SSS a', second: 'h:mm:ss a', minute: 'h:mm a', @@ -1570,6 +1515,7 @@ describe('Time scale tests', function() { // NOTE: built-in adapter uses moment var expected = { + datetime: 'MMM D, YYYY, h:mm:ss a', millisecond: 'foo', second: 'h:mm:ss a', minute: 'h:mm a', From 03e6178564d6fac27fc94c5f58023778e0caa378 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Thu, 14 Feb 2019 15:35:52 -0800 Subject: [PATCH 5/6] Update docs --- src/core/core.adapters.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/core.adapters.js b/src/core/core.adapters.js index 8f61fbcc235..e492fd68e92 100644 --- a/src/core/core.adapters.js +++ b/src/core/core.adapters.js @@ -30,7 +30,8 @@ function abstract() { /** @lends Chart._adapters._date */ module.exports._date = { /** - * Returns a map of time formats for the supported units. + * Returns a map of time formats for the supported formatting units. + * This includes all scale tick units defined in Unit as well as 'datetime' used in the tooltip. * @returns {{string: string}} */ formats: abstract, From 4c85e4b5f643bc467e3c5d9d1d7cd791fd731701 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Mon, 18 Feb 2019 10:26:31 -0800 Subject: [PATCH 6/6] Update comment --- src/core/core.adapters.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/core.adapters.js b/src/core/core.adapters.js index e492fd68e92..246f3b70d3e 100644 --- a/src/core/core.adapters.js +++ b/src/core/core.adapters.js @@ -30,8 +30,8 @@ function abstract() { /** @lends Chart._adapters._date */ module.exports._date = { /** - * Returns a map of time formats for the supported formatting units. - * This includes all scale tick units defined in Unit as well as 'datetime' used in the tooltip. + * Returns a map of time formats for the supported formatting units defined + * in Unit as well as 'datetime' representing a detailed date/time string. * @returns {{string: string}} */ formats: abstract,