Implemented aligment by senior unit in time axis. #4267

hurskiy-andriy · 2017-05-17T07:18:36Z

Time axis ticks are aligned by senior units of determined base units (for example: if units determined as "hour", senior units will be "day"). See #4187
Check samples:
samples/scales/time/combo.html
and
samples/scales/time/line-point-data.html

benmccann · 2017-05-17T15:35:35Z

src/scales/scale.time.js

@@ -315,15 +331,26 @@ module.exports = function(Chart) {

 			var maxTicks = me.getLabelCapacity(minTimestamp || dataMin);
 			var unit = timeOpts.unit || determineUnit(timeOpts.minUnit, minTimestamp || dataMin, maxTimestamp || dataMax, maxTicks);
+			var units = Object.keys(interval);
+			var seniorUnit = null;


I'd probably call it majorUnit vs seniorUnit

Wll be changed in next pull request

@hurskiy-andriy can you change that in this PR?

Sorry. Changed in this PR.

benmccann · 2017-05-17T15:37:58Z

test/specs/scale.time.tests.js

@@ -218,7 +218,7 @@ describe('Time scale tests', function() {

 		// Counts down because the lines are drawn top to bottom
 		expect(xScale.ticks[0]).toEqualOneOf(['Nov 19, 1981', 'Nov 20, 1981', 'Nov 21, 1981']); // handle time zone changes
-		expect(xScale.ticks[1]).toEqualOneOf(['Nov 19, 1981', 'Nov 20, 1981', 'Nov 21, 1981']); // handle time zone changes
+		// expect(xScale.ticks[1]).toEqualOneOf(['Nov 19, 1981', 'Nov 20, 1981', 'Nov 21, 1981']); // handle time zone changes


can this be changed so that it passes? e.g. by changing the expected values to 'Dec 1, 1981' or whatever it is that it's expecting now
or if there's not a reasonable way to fix it, it's probably better to delete it rather than leaving commented out code

Uncommented and changed in next pull request

@hurskiy-andriy same here!

Changed in this PR

Uncommented in last commit

etimberg · 2017-05-17T22:24:57Z

src/scales/scale.time.js

@@ -315,15 +331,26 @@ module.exports = function(Chart) {

 			var maxTicks = me.getLabelCapacity(minTimestamp || dataMin);
 			var unit = timeOpts.unit || determineUnit(timeOpts.minUnit, minTimestamp || dataMin, maxTimestamp || dataMax, maxTicks);
+			var units = Object.keys(interval);
+			var seniorUnit = null;


etimberg · 2017-05-17T22:26:06Z

src/scales/scale.time.js

-			ticks.push(options.min !== undefined ? options.min : niceRange.min);
-			var cur = moment(niceRange.min);
+			var startTick = options.min !== undefined ? options.min : niceRange.min;
+			var seniorUnitStart = moment(startTick).add(1, options.seniorUnit).startOf(options.seniorUnit);


what will happen if options.seniorUnit is not set? That would be the default case

If options.seniorUnit is not set - there will be no alignment by majorUnit.
Added check of options.seniorUnit

We discussed in the feature request #4187 that the alignment always be on. Can you auto-detect what the majorUnit is? You should be able to use determineUnit and interval to do so. I think this would be much simpler for the user

majorUnit is auto-detected on line #340 and then passes to options on line #353

Can we remove options.majorUnit? I don't think the user should be able to change it as it just adds additional complexity

These options are not user defined options. This is object constructed on line #347 and passed to Chart.Ticks.generators.time call and the inside this call passed to generateTicks function at line #231

Yes, you're correct. Thanks for clarifying

benmccann · 2017-05-18T15:36:36Z

test/specs/scale.time.tests.js

@@ -231,7 +232,7 @@ describe('Time scale tests', function() {

 		var scale = createScale(mockData, config);
 		scale.update(2500, 200);
-		expect(scale.ticks).toEqual(['Jan 1, 8PM', 'Jan 1, 9PM', 'Jan 1, 10PM', 'Jan 1, 11PM', 'Jan 2, 12AM', 'Jan 2, 1AM', 'Jan 2, 2AM', 'Jan 2, 3AM', 'Jan 2, 4AM', 'Jan 2, 5AM', 'Jan 2, 6AM', 'Jan 2, 7AM', 'Jan 2, 8AM', 'Jan 2, 9AM', 'Jan 2, 10AM', 'Jan 2, 11AM', 'Jan 2, 12PM', 'Jan 2, 1PM', 'Jan 2, 2PM', 'Jan 2, 3PM', 'Jan 2, 4PM', 'Jan 2, 5PM', 'Jan 2, 6PM', 'Jan 2, 7PM', 'Jan 2, 8PM', 'Jan 2, 9PM']);
+		expect(scale.ticks).toEqual(['8PM', '9PM', '10PM', '11PM', 'Jan 2', '1AM', '2AM', '3AM', '4AM', '5AM', '6AM', '7AM', '8AM', '9AM', '10AM', '11AM', '12PM', '1PM', '2PM', '3PM', '4PM', '5PM', '6PM', '7PM', '8PM', '9PM']);


There should either be one PR with both the alignment and formatting changes. Or one PR with only alignment changes and one PR with only formatting changes.

This PR says it's the alignment changes and that #4268 is the formatting changes. But you're making formatting changes here which is really confusing.

benmccann · 2017-05-19T17:04:38Z

src/scales/scale.time.js

+			var alignedTick = startTick;
+			ticks.push(startTick);
+			if (startTick !== alignedTick + startFraction &&
+				options.majorUnit &&


Can you indent continuation lines +2 tabs? I.e. this line and the next two lines. That way it's clearer what's part of the if and what's inside it

I don't think that will make ESLint happy!

Ok, well if it makes ESLint fail, then nevermind. @simonbrunel do you have any convention about whether the && should be at the end of the line or the beginning of the next line?

I'm used to && and || at the end but it's not enforced by the linter.

This code should fit in one line and be more readable:

startTick !== alignedTick + startFraction eq. startFraction !== 0

majorUnit should be cached because it's used many time

same for timeOpts

var majorUnit = options.majorUnit; var timeOpts = options.timeOpts; //... if (majorUnit && startFraction && !timeOpts.round && !timeOpts.isoWeekday) { // ... }

benmccann · 2017-05-19T17:10:06Z

src/scales/scale.time.js

 				unit: unit,
+				timeOpts: timeOpts,


it's weird that here we pass in timeOpts and on the next line we pass in timeOpts.isoWeekday. we should maybe change this to timeOpts.round for consistency?

benmccann · 2017-05-19T17:10:38Z

src/scales/scale.time.js

 			me.displayFormat = timeOpts.displayFormats[unit];
+			me.seniorDisplayFormat = timeOpts.displayFormats[majorUnit];


we should probably call this majorDisplayFormat for consistency

benmccann · 2017-05-26T13:20:25Z

src/scales/scale.time.js

+			var startTick = options.min !== undefined ? options.min : niceRange.min;
+			var majorUnitStart = startTick;
+			if (options.majorUnit) {
+				majorUnitStart = moment(startTick).add(1, options.majorUnit).startOf(options.majorUnit);


if startTick is already at a majorUnit then I think you'll end up at the next one. do you really want something like:

majorUnitStart = moment(startTick).startOf(options.majorUnit) == moment(startTick) ? moment(startTick) : moment(startTick).add(1, options.majorUnit).startOf(options.majorUnit);

Assignment startTick to majorUnitStart on line #179 is performed instead of else block of if statement at line #180
In other words we can write:
var majorUnitStart; if (options.majorUnit) { majorUnitStart = moment(startTick).add(1, options.majorUnit).startOf(options.majorUnit); } else { majorUnitStart = startTick; }

Do we want to be rounding up here? I would have guessed we'd round down. In which case we wouldn't add 1 and you could ignore my previous comment

benmccann · 2017-05-26T13:23:31Z

src/scales/scale.time.js

+			var startFraction = startRange % (interval[options.unit].size * stepSize);
+			var alignedTick = startTick;
+			ticks.push(startTick);
+			if (startTick !== alignedTick + startFraction && options.majorUnit && !options.timeOpts.round && !options.timeOpts.isoWeekday) {


I'm not sure why you need startTick !== alignedTick + startFraction. Is this equivalent to startTick !== majorUnitStart?

For sure, it's equivalent to if (startFraction && ... (see my previous comment)

If startTick is not equal to time that aligned by stepSize accordingly to majorUnit we need to find first aligned tick so then we can push ticks with stepSize and there will be ticks aligned to majorUnit.
In other words if we have:
startTick == 20:12;
majorUnit == "hour";
sterSize == "3 hour";
majorUnitStart will equal to "00:00", first aligned tick will equal to "21:00" and startFraction will equal to "48 minutes".
With this value of startFraction we can determine alignedTick (tick which aligned by stepSize accordingly to majorUnit (21:00, 00:00, 03:00 etc..))
When we perform startTick !== alignedTick + startFraction at line #187 we check if we need to push aligned tick (21:00) or startTick (20:12) is already aligned.
So startFraction is not difference between startTick (20:12) and majorUnitStart (00:00) but difference between startTick (20:12) and first aligned tick (21:00)

@hurskiy-andriy if var alignedTick = startTick; (line 185) then:

if (startTick !== alignedTick + startFraction && ... eq.
if (startTick !== startTick + startFraction && ... eq.
if (startFraction !== 0 && ... eq.
if (startFraction && ...

Am I right?

@simonbrunel yes

@hurskiy-andriy I'm not following your example. I don't understand why majorUnitStart will be equal to "00:00". Wouldn't it be equal to "21:00"?

majorUnitStart = moment(startTick).add(1, options.majorUnit).startOf(options.majorUnit);
majorUnitStart = (20.12 + 1hr).startOf(hour) = 21.12.startOf(hour) = 21:00

@benmccann Sorry, my mistake. In example above unit == "hour" and majorUnit == "day"

benmccann · 2017-05-26T13:25:13Z

@hurskiy-andriy can you rebase both your PRs?

hurskiy-andriy · 2017-05-26T15:02:11Z

@benmccann rebase master into my branches?

…ck 'if' statement formatting. Removed isoWeekday from generateTick ooptions due to redundancy.

benmccann · 2017-05-26T18:09:16Z

Yes, rebase each branch against master, so that they can be merged. It looks like you've already done that

benmccann · 2017-05-27T14:34:18Z

The samples (e.g. samples/scales/time/line-point-data.html and samples/scales/time/combo.html) look much nicer with this change. The one improvement I would suggest is to also align the lefthand and righthand side of the chart just as we do for the points on the interior of the chart. E.g. on samples/scales/time/line-point-data.html it would be nice to make the lefthand side "May 27, 6AM" and the righthand side "Jun 1, 12PM"

Doing so would also fix some existing bugs such as #4293

simonbrunel · 2017-05-27T14:55:21Z

src/helpers/helpers.time.js

-			var cur = moment(niceRange.min);
+			var startTick = options.min !== undefined ? options.min : niceRange.min;
+			var majorUnitStart = startTick;
+			if (options.majorUnit) {


options.majorUnit is used quite often in this method, I would cache it (var majorUnit = options.majorUnit) to increase minification.

… first and last ticks to time steps.

benmccann · 2017-05-29T15:03:44Z

test/specs/scale.time.tests.js

@@ -94,7 +94,7 @@ describe('Time scale tests', function() {
 				displayFormat: false,
 				minUnit: 'millisecond',
 				displayFormats: {
-					millisecond: 'h:mm:ss.SSS a', // 11:20:01.123 AM
+					millisecond: 'h:mm:ss.SSS a', // 11:20:01.123 AM,


don't need a comma at the end of this line

benmccann · 2017-05-29T15:09:00Z

src/helpers/helpers.time.js

+			var majorUnitStart = startTick;
+			var majorUnit = options.majorUnit;
+			if (majorUnit) {
+				majorUnitStart = moment(startTick).add(1, majorUnit).startOf(majorUnit);


you could combine a few lines by doing:

var majorUnitStart = majorUnit ? moment(startTick).add(1, majorUnit).startOf(majorUnit) : startTick;

benmccann · 2017-05-29T15:12:31Z

src/helpers/helpers.time.js

+			var startFraction = startRange % stepValue;
+			var alignedTick = startTick;
+			if (startFraction && majorUnit && !options.timeOpts.round && !options.timeOpts.isoWeekday) {
+				alignedTick += startFraction - stepValue;


Is alignedTick += startFraction - stepValue equal to alignedTick -= startFraction ? If so, that might be a more straightforward way of writing it

I startFraction will equal to 0 - we haven't pass condition on line above

I'm not sure how that's related to my question...

benmccann · 2017-05-29T15:14:28Z

Thanks @hurskiy-andriy This looks great! The samples render so nicely now :-)

I left just a couple very small comments, but otherwise looks good to me

benmccann · 2017-05-29T15:37:53Z

src/helpers/helpers.time.js

-			if (majorUnit) {
-				majorUnitStart = moment(startTick).add(1, majorUnit).startOf(majorUnit);
-			}
+			var majorUnitStart = majorUnit ? majorUnitStart = moment(startTick).add(1, majorUnit).startOf(majorUnit) : startTick;


This line has majorUnitStart = twice. You only need it once:

var majorUnitStart = majorUnit ? moment(startTick).add(1, majorUnit).startOf(majorUnit) : startTick;

Implemented alignment by major unit in the time scale. This allows showing the first tick of a larger unit like days in a special way and is part of the basis of the time series scale.

benmccann reviewed May 17, 2017

View reviewed changes

benmccann mentioned this pull request May 17, 2017

Time axis ticks formatting for base and senior unit #4268

Merged

etimberg reviewed May 17, 2017

View reviewed changes

benmccann reviewed May 18, 2017

View reviewed changes

benmccann reviewed May 19, 2017

View reviewed changes

benmccann reviewed May 26, 2017

View reviewed changes

andrew.hurskiy added 6 commits May 26, 2017 18:43

Implemented aligment by senior unit in time axis.

4888830

Changed seniorUnit to majorUnit. Added majorUnit check.

a47c7e3

Time scale majorUnit alignment fixes.

90eeedd

Scale time tests fix.

9d90575

Renamed seniorDisplayFormat to majorDisplayFormat. Changed aligned ti…

a6fba63

…ck 'if' statement formatting. Removed isoWeekday from generateTick ooptions due to redundancy.

Added major tick determining in timeHelpers.

deee58f

hurskiy-andriy force-pushed the border-time-axis-ticks branch from 76a3d19 to deee58f Compare May 26, 2017 17:11

etimberg approved these changes May 26, 2017

View reviewed changes

etimberg added this to the Version 2.7 milestone May 27, 2017

simonbrunel reviewed May 27, 2017

View reviewed changes

Added caching of options.majorUnit at timeHelpers. Added alignment of…

d7424cf

… first and last ticks to time steps.

benmccann reviewed May 29, 2017

View reviewed changes

Minor fixes.

3f39647

benmccann reviewed May 29, 2017

View reviewed changes

Removed redundant majorUnitStart assignment at timeHelpers.

abe456f

etimberg merged commit f7f177f into chartjs:master May 29, 2017

simonbrunel added the type: enhancement label May 30, 2017

etimberg mentioned this pull request Jul 17, 2017

[BUG] master branch time scale min/max handling broken #4514

Closed

		me.displayFormat = timeOpts.displayFormats[unit];
		me.seniorDisplayFormat = timeOpts.displayFormats[majorUnit];

Implemented aligment by senior unit in time axis. #4267

Implemented aligment by senior unit in time axis. #4267

Conversation

hurskiy-andriy commented May 17, 2017

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 May 17, 2017 • edited

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

hurskiy-andriy May 18, 2017 • edited

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

simonbrunel May 19, 2017 • edited

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 May 26, 2017 • edited

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 May 26, 2017

hurskiy-andriy commented May 26, 2017 • edited

benmccann commented May 26, 2017 • edited

benmccann commented May 27, 2017

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 May 29, 2017

Choose a reason for hiding this comment

benmccann May 17, 2017 •

edited

hurskiy-andriy May 18, 2017 •

edited

simonbrunel May 19, 2017 •

edited

benmccann May 26, 2017 •

edited

hurskiy-andriy commented May 26, 2017 •

edited

benmccann commented May 26, 2017 •

edited