Time axis ticks formatting for base and senior unit by hurskiy-andriy · Pull Request #4268 · chartjs/Chart.js

hurskiy-andriy · 2017-05-17T13:49:52Z

Changed default formatting for time units.
Bold style added to senior unit tick labels.

etimberg · 2017-05-17T22:27:30Z

src/core/core.scale.js

 					context.rotate(itemToDraw.rotation);
-					context.font = tickFont.font;
+					var font = tickFont.font;
+					if (me.ticksAsTimestamps) {


Not sure that adding in this detail the core scale is the best way to go

benmccann · 2017-05-29T19:40:04Z

src/scales/scale.time.js

@@ -26,8 +26,8 @@ module.exports = function(Chart) {
 				millisecond: 'h:mm:ss.SSS a', // 11:20:01.123 AM,
 				second: 'h:mm:ss a', // 11:20:01 AM
 				minute: 'h:mm:ss a', // 11:20:01 AM


I think we should get rid of :ss on this line

benmccann · 2017-05-29T19:41:09Z

src/scales/scale.time.js

-				day: 'll', // Sep 4 2015
+				hour: 'hA', // 5PM
+				day: 'MMM D', // Sep 4
 				week: 'll', // Week 46, or maybe "[W]WW - YYYY" ?


I think we should change this to MMM D as well

benmccann · 2017-05-29T23:13:08Z

@hurskiy-andriy could you rebase this PR against master?

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

benmccann · 2017-05-30T16:21:35Z

I think that the main thing to address is etimberg's comment about modifying core.scale.js

The other thing I was going to mention was samples/scales/time/line.html. There are sort of weird dates that are bolded. I'm guessing this is because the majorUnit is week. Week and quarter don't make a lot of sense to me as being units people commonly refer to, so I would suggest we only use month and year as majorUnit

simonbrunel

It seems this PR forces a specific design for the "major" tick (bold), right? is it a breaking change? I'm not sure everyone will want it that way, at least the font should be fully customizable?

simonbrunel · 2017-05-30T17:04:59Z

src/core/core.scale.js

 'use strict';

+var moment = require('moment');
+moment = typeof(moment) === 'function' ? moment : window.moment;


Agree with @etimberg's comment, Moment should not leak in core.scale.js but stays confined in scale.time.js

benmccann · 2017-05-30T17:38:29Z

I agree that having an option to control the formatting makes sense. That's what was discussed in issue #4187 describing the feature as well.

This is probably a good default to have. Here are some screenshots of the before and after. I think it's much cleaner and easier to digest the x-axis with this change.

Here's a sample from before:

Here's a sample with the changes:

benmccann · 2017-05-31T03:39:01Z

@etimberg @simonbrunel I've been reviewing @hurskiy-andriy's change and trying to figure out how to remove moment from core.scale.js. I think the cleanest way to do this is to introduce the concept of major and minor ticks so that core.scale.js can draw them each differently

Right now all the ticks are stored in the ticks variable. Should we have a minorTicks and majorTicks and remove ticks? Or should we make ticks store an object like {value:123, major:true}? Other thoughts or suggestions?

etimberg · 2017-05-31T10:35:00Z

I don't think we can just remove the ticks property since other stuff might use it. I'm ok with adding the concept of major ticks to the axis and storing them separately.

hurskiy-andriy · 2017-05-31T12:53:51Z

Changed ticks store to array of objects like `{value: '12AM', major: true}

etimberg

@simonbrunel do you think the changes to the values in the scale.ticks property would be considered a breaking change?

etimberg · 2017-06-02T10:56:16Z

src/scales/scale.time.js

+
 			Chart.Scale.prototype.initialize.call(this);
 		},
+		mergeTicksOptions: function() {


The docs for this function imply that it should be applicable to all scales. Should mergeTicksOptions be moved to the base scale class?

Also, we should really mark all internal members private and make them public only if requested.

Sorry I can't find docs for mergeTicksOptions. Goal of this function is to fill majorTicks options with default options. Should I rename this function to fillTimeTicksOptions?

This method should apply to all scale, so the new major options can be used by other scale implementations. That's why this merging logic should be in the base core.scale.js.

Ok understood. Thanks!

simonbrunel · 2017-06-02T12:42:23Z

src/scales/scale.time.js

 		ticks: {
 			autoSkip: false
+		},
+		majorTicks: {


I liked to have all tick options scoped under the ticks key, but having it duplicated under majorTicks doesn't look ideal but also not obvious that it overrides the ticks options. Not sure either about this alternative, but what do you think about:

ticks: { // common tick options // ... minor: { // overrides for minor ticks only }, major: { // overrides for major ticks only } }

ticks.minor: false: don't show minor ticks
ticks.major: false: don't show major ticks

Also, defaults major tick options should be major: {}, no need to duplicate values if exactly the same, the merge process will take care of that (same for minor).

If minor ticks will fetch their options from options.ticks.minor I think it will be breaking change or at least we'll must to refactor all other scales to use options.ticks.minor options.
I suggest to use your variant but without minor key. In that case minor ticks will use common ticks options and major ticks will use major options. So ticks options will looks like:
ticks: { // common tick options // ... major: { // overrides for major ticks only } }

I mean, global options for ticks.minor and ticks.major would be empty objects:

ticks: { minor: {}, //< use common tick options major: {} //< use common tick options }

and we keep current global ticks options so when computing effective minor and major options, it would fallback to ticks global options:

(effective) minor options = deep merge(ticks, tick.minor)

(effective) major options = deep merge(ticks, tick.major)

It's not a breaking change because minor and major ticks would use the ticks values by default, same as what we have in 2.6. Also having the shorthand ticks.minor: false (same for major) to hide minor or major ticks would be great.

@etimberg @benmccann thoughts?

Yes, @simonbrunel, I like your suggestion

@simonbrunel I like that suggestion as well 😄

simonbrunel · 2017-06-02T12:44:51Z

@etimberg if scale.ticks are used by plugins, then it's a breaking change!

benmccann · 2017-06-02T21:37:36Z

I just did an audit of all the charts and plugins under the chartjs group on GitHub. None are using scale.ticks besides chartjs-chart-financial. Don't worry about chartjs-chart-financial though because it hasn't published an initial release yet, and I'll take responsibility for updating it before then.

Interestingly I discovered that the linear gauge chart also has the concept of major and minor ticks and has majorTicks and minorTicks options. I like the syntax we came up with better than the syntax it uses, so I think we should still use ticks.major and ticks.minor and update the linear gauge chart accordingly. I filed an issue in that repo to track

simonbrunel · 2017-06-04T17:00:46Z

@benmccann thanks, so it seems fine to change the internal structure of scale.ticks, could be interesting to add a note in the code that make it private explicitly. I would not really care about options consistency with the LinearGauge chart because it's still based on Chart.js 1.x.

etimberg · 2017-06-07T11:32:25Z

@simonbrunel do you think this can be merged?

simonbrunel · 2017-06-07T12:35:17Z

It depends if we want to change how the options are exposed ({ticks: {minor: {}, major: {}, ... common ... })

benmccann · 2017-06-07T17:01:48Z

Yes, let's update that first. @hurskiy-andriy would you be able to update that before we merge this PR?

hurskiy-andriy · 2017-06-08T12:14:29Z

Changed ticks storage back to array of string. Major ticks are determined through majorTicksIndexes array.

benmccann · 2017-06-08T17:04:32Z

Thanks for changing the config options.

I'm curious why did you change the ticks storage back to an array of strings? I liked it better when you just had ticks instead of ticks and majorTicksIndexes. Is it possible to change it back?

hurskiy-andriy · 2017-06-09T07:01:37Z

@benmccann

If scale.ticks are used by plugins, then it's a breaking change!
So I change ticks back to array of strings for compatibility.

hurskiy-andriy · 2017-06-11T17:03:07Z

@benmccann @simonbrunel
I decided to revert back to array of string because the simonbrunel noticed in #4268 (comment)
Let's come to agreement what format should we use

benmccann · 2017-06-11T20:06:16Z

@hurskiy-andriy he said afterwards in #4268 (comment):

thanks, so it seems fine to change the internal structure of scale.ticks

hurskiy-andriy · 2017-06-12T07:32:39Z

Changed ticks storage to array of objects like {value: '12 PM', major: true}

simonbrunel

Looks good, just a few minification notes

simonbrunel · 2017-06-12T08:17:44Z

src/core/core.scale.js

 		// Any function can be extended by the scale type

+		mergeTicksOptions: function() {
+			if (this.options.ticks.minor === false) {


Minification: (this.options.ticks is used 11 times)

var ticks = this.options.ticks; if (ticks.minor === false) { // ...

simonbrunel · 2017-06-12T08:17:47Z

src/core/core.scale.js

 			var globalDefaults = Chart.defaults.global;
-			var optionTicks = options.ticks;
+			var optionTicks = options.ticks.minor;
+			var optionMajorTicks = options.ticks.major ? options.ticks.major : optionTicks;


var optionMajorTicks = options.ticks.major || optionTicks;

simonbrunel · 2017-06-12T08:17:59Z

src/core/core.scale.js


-			helpers.each(me.ticks, function(label, index) {
+			helpers.each(me.ticks, function(tick, index) {
+				var label = typeof tick === 'object' && typeof tick.value !== 'undefined' ? tick.value : tick;


var label = (tick && tick.value) || tick;

simonbrunel · 2017-06-12T08:18:16Z

src/core/core.scale.js

 					glBorderDashOffset: borderDashOffset,
 					rotation: -1 * labelRotationRadians,
 					label: label,
+					major: tick.major === true,


Why not simply: major: tick.major?

simonbrunel

Thanks @hurskiy-andriy. I didn't test this patch, if anyone else wants to checkout this PR and make a few extra checks before merging :)

benmccann · 2017-06-13T04:21:55Z

I tested this locally. It mostly looks good. The one thing I would change is that I would remove week and quarter from interval. It's strange to have alignment on a week. E.g. I have an example that's aligning on July 2, 2017. I'm guessing that's the start of week 26 of the year or something, but really I want it aligned on July 1, 2017 since that's the start of the month.

hurskiy-andriy · 2017-06-13T07:23:54Z

'week' and 'quarter' time units are prevented from determining as major units. Instead 'month' (if minor unit == 'day') or 'year' (if minor unit == 'month') unit will be determined as major.

etimberg · 2017-06-13T11:13:09Z

So this is good to merge?

benmccann · 2017-06-13T14:45:25Z

Thanks for the update @hurskiy-andriy

@etimberg @simonbrunel I think we should be good to merge this one now

Working towards creating the TimeSeries scale, this PR adds formatting for major and minor ticks on axes.

etimberg reviewed May 17, 2017

View reviewed changes

benmccann mentioned this pull request May 18, 2017

Implemented aligment by senior unit in time axis. #4267

Merged

hurskiy-andriy force-pushed the time-axis-ticks-formatting branch from 0e0c643 to e2b687b Compare May 26, 2017 17:39

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

benmccann reviewed May 29, 2017

View reviewed changes

andrew.hurskiy added 11 commits May 30, 2017 10:03

Resolved conflict on time-axis-ticks-formatting rebase.

0fc6ed3

Changed seniorUnit to majorUnit. Added majorUnit check.

0572606

Time scale majorUnit alignment fixes.

890f1c5

Scale time tests fix.

f5ddc57

Renamed seniorDisplayFormat to majorDisplayFormat. Changed aligned ti…

d43f9ee

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

Added major tick determining in timeHelpers.

b471c73

Added majorTick formatting.

d88a03a

Minor fixes.

dd66acb

Changed time unit formatting. Minor fixes.

86c4259

Added import of moment into core.scale.js.

241ad82

Time scale testts fix.

086bf49

hurskiy-andriy force-pushed the time-axis-ticks-formatting branch from db3bdbd to 086bf49 Compare May 30, 2017 07:19

simonbrunel requested changes May 30, 2017

View reviewed changes

Changed time scale ticks store to array of objects.

7242f96

Added majorTick configuration via options.

5bd9cb6

etimberg approved these changes Jun 2, 2017

View reviewed changes

simonbrunel requested changes Jun 2, 2017

View reviewed changes

benmccann mentioned this pull request Jun 2, 2017

Utilize same major and minor tick settings as core library chartjs/Chart.LinearGauge.js#7

Open

Changed minor and major ticks configuration in ticks options.

3ab65fc

hurskiy-andriy force-pushed the time-axis-ticks-formatting branch from 05cf849 to 5bd9cb6 Compare June 12, 2017 07:27

simonbrunel requested changes Jun 12, 2017

View reviewed changes

Minification changes.

7bcae92

simonbrunel approved these changes Jun 12, 2017

View reviewed changes

Excluded 'week' and 'quarter' units from determining as major.

0dd966f

etimberg mentioned this pull request Jun 14, 2017

Huge gap between last two labels when using maxTicksLimit #4358

Closed

etimberg merged commit 2d7c1f0 into chartjs:master Jun 15, 2017

mMerlin mentioned this pull request Jul 14, 2017

[BUG] Tick text extents not calculated correctly #4502

Closed

etimberg mentioned this pull request Jul 17, 2017

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

Closed

simonbrunel mentioned this pull request Jul 27, 2017

Change scale.ticks back to an array of strings #4573

Merged

simonbrunel added the type: enhancement label Aug 14, 2017

moberwasserlechner mentioned this pull request May 30, 2018

Add minor/major configs to Ticks moberwasserlechner/vaadin-chartjs#77

Open

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021

Time axis tick formatting with major and minor units (chartjs#4268)

00aa210

Working towards creating the TimeSeries scale, this PR adds formatting for major and minor ticks on axes.

Conversation

hurskiy-andriy commented May 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann commented May 29, 2017

Uh oh!

benmccann commented May 30, 2017

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann commented May 30, 2017

Uh oh!

benmccann commented May 31, 2017

Uh oh!

etimberg commented May 31, 2017

Uh oh!

hurskiy-andriy commented May 31, 2017

Uh oh!

etimberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonbrunel commented Jun 2, 2017

Uh oh!

benmccann commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonbrunel commented Jun 4, 2017

Uh oh!

etimberg commented Jun 7, 2017

Uh oh!

simonbrunel commented Jun 7, 2017

Uh oh!

benmccann commented Jun 7, 2017

Uh oh!

hurskiy-andriy commented Jun 8, 2017

Uh oh!

benmccann commented Jun 8, 2017

Uh oh!

hurskiy-andriy commented Jun 9, 2017

Uh oh!

hurskiy-andriy commented Jun 11, 2017

Uh oh!

benmccann commented Jun 11, 2017

Uh oh!

hurskiy-andriy commented Jun 12, 2017

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

benmccann commented Jun 2, 2017 •

edited

Loading