New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Time axis ticks formatting for base and senior unit #4268
Time axis ticks formatting for base and senior unit #4268
Conversation
src/core/core.scale.js
Outdated
@@ -681,7 +681,17 @@ module.exports = function(Chart) { | |||
context.save(); | |||
context.translate(itemToDraw.labelX, itemToDraw.labelY); | |||
context.rotate(itemToDraw.rotation); | |||
context.font = tickFont.font; | |||
var font = tickFont.font; | |||
if (me.ticksAsTimestamps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that adding in this detail the core scale is the best way to go
0e0c643
to
e2b687b
Compare
src/scales/scale.time.js
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should get rid of :ss
on this line
src/scales/scale.time.js
Outdated
hour: 'MMM D, hA', // Sept 4, 5PM | ||
day: 'll', // Sep 4 2015 | ||
hour: 'hA', // 5PM | ||
day: 'MMM D', // Sep 4 | ||
week: 'll', // Week 46, or maybe "[W]WW - YYYY" ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change this to MMM D
as well
@hurskiy-andriy could you rebase this PR against master? |
…ck 'if' statement formatting. Removed isoWeekday from generateTick ooptions due to redundancy.
db3bdbd
to
086bf49
Compare
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
src/core/core.scale.js
Outdated
@@ -1,5 +1,8 @@ | |||
'use strict'; | |||
|
|||
var moment = require('moment'); | |||
moment = typeof(moment) === 'function' ? moment : window.moment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@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 |
I don't think we can just remove the |
Changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonbrunel do you think the changes to the values in the scale.ticks
property would be considered a breaking change?
src/scales/scale.time.js
Outdated
Chart.Scale.prototype.initialize.call(this); | ||
}, | ||
mergeTicksOptions: function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs for this function imply that it should be applicable to all scales. Should mergeTicksOptions
be moved to the base scale class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should really mark all internal members private and make them public only if requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok understood. Thanks!
src/scales/scale.time.js
Outdated
@@ -36,6 +36,19 @@ module.exports = function(Chart) { | |||
}, | |||
ticks: { | |||
autoSkip: false | |||
}, | |||
majorTicks: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 } }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etimberg @benmccann thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @simonbrunel, I like your suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonbrunel I like that suggestion as well 😄
@etimberg if |
I just did an audit of all the charts and plugins under the chartjs group on GitHub. None are using Interestingly I discovered that the linear gauge chart also has the concept of major and minor ticks and has |
@benmccann thanks, so it seems fine to change the internal structure of |
@simonbrunel do you think this can be merged? |
It depends if we want to change how the options are exposed ( |
Yes, let's update that first. @hurskiy-andriy would you be able to update that before we merge this PR? |
Changed ticks storage back to array of string. Major ticks are determined through |
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 |
|
@benmccann @simonbrunel |
@hurskiy-andriy he said afterwards in #4268 (comment):
|
05cf849
to
5bd9cb6
Compare
Changed ticks storage to array of objects like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few minification notes
src/core/core.scale.js
Outdated
@@ -94,6 +96,28 @@ module.exports = function(Chart) { | |||
// Any function defined here is inherited by all scale types. | |||
// Any function can be extended by the scale type | |||
|
|||
mergeTicksOptions: function() { | |||
if (this.options.ticks.minor === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minification: (this.options.ticks
is used 11 times)
var ticks = this.options.ticks;
if (ticks.minor === false) {
// ...
src/core/core.scale.js
Outdated
@@ -486,7 +510,8 @@ module.exports = function(Chart) { | |||
|
|||
var context = me.ctx; | |||
var globalDefaults = Chart.defaults.global; | |||
var optionTicks = options.ticks; | |||
var optionTicks = options.ticks.minor; | |||
var optionMajorTicks = options.ticks.major ? options.ticks.major : optionTicks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var optionMajorTicks = options.ticks.major || optionTicks;
src/core/core.scale.js
Outdated
@@ -547,7 +571,8 @@ module.exports = function(Chart) { | |||
var yTickStart = options.position === 'bottom' ? me.top : me.bottom - tl; | |||
var yTickEnd = options.position === 'bottom' ? me.top + tl : me.bottom; | |||
|
|||
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var label = (tick && tick.value) || tick;
src/core/core.scale.js
Outdated
@@ -645,6 +670,7 @@ module.exports = function(Chart) { | |||
glBorderDashOffset: borderDashOffset, | |||
rotation: -1 * labelRotationRadians, | |||
label: label, | |||
major: tick.major === true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply: major: tick.major
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
I tested this locally. It mostly looks good. The one thing I would change is that I would remove |
'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. |
So this is good to merge? |
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.
Changed default formatting for time units.
Bold style added to senior unit tick labels.