Format the label in the time scale tooltip

[benmccann]

@simonbrunel this is an alternative implementation of #4583

Before

After

[simonbrunel]

It would be better to move the calculation of the label format string into a separate function, then call this function only one time at the end of determineDataLimits or buildTicks and cache it in a private variable:

  // ...
  this._format = timeOpts.tooltipFormat || determineLabelFormat(...);

This variable should then be used in getLabelForIndex

getLabelForIndex: function(index) {
  var me = this;
  var data = me.chart.data;
  var label = data.labels && index < data.labels.length ? data.labels[index] : '';
  return momentify(label, me.options.time).format(me._format);
}

[benmccann]

@simonbrunel thanks for the review! I've updated this PR

[simonbrunel]

Does that mean that currently (master), the default tooltip label is the timestamp? Are there any issues to link to that PR?

[benmccann]

Thanks. I updated it

The current master doesn't do any formatting by default. I see a timestamp because that's what I pass in. Other users might pass in something else like a moment object, but it still won't be formatted. I did not open an issue for it - only a PR

[simonbrunel]

If your data is a formatted string (e.g. 2018-01-07), it would have been displayed as is, but now it would be changed for another hard coded format (2018-01-07 00:00:00), unless the user specify a tooltipFormat? If so, isn't it a breaking change?

[simonbrunel]

I didn't mean you should have created an issue for this PR, but rather I'm wondering if this bug hasn't been reported by other users and so we would be able to close them with that PR.

[benmccann]

Ok, I added an extra check to just use the string if it's provided as a string.

I'm not aware of any other users reporting this issue. I think it's more common to encounter since the labels.source: 'data' which is a pretty recent feature

[simonbrunel]

Could be related to old #1466

[simonbrunel]

Looks good, I would add a few unit tests for this feature.

[benmccann]

I've added unit tests

[simonbrunel]

We should test all cases: 'MMM D, YYYY h:mm:ss.SSS a', 'MMM D, YYYY h:mm:ss a' and 'MMM D, YYYY', but also preserving a string.

[etimberg]

what happens at 00:00:00 ie, midnight on a day? Or is there no way to detect a time set with that from a moment object that has no time at all?

[benmccann]

The chart.js user is specifying timestamps if we're utilizing this function. With timestamps there's no way to differentiate dates vs date plus time of midnight. However, if every single timestamp in the dataset is midnight then it's relatively clear that they're representing dates. Also, I don't know that there's any practical difference between date vs date+midnight.

[simonbrunel]

True, if there is at least one time other than midnight, then it will pick 'MMM D, YYYY h:mm:ss a', else if all times are midnight then it will pick 'MMM D, YYYY', which is ok I guess.

[simonbrunel]

@etimberg Good point and I'm wondering again if it would not be easier to simply use by default the same format as the scale tick label as asked in #1466?

@benmccann I know that you are trying to show more detailed information, but it's one use case and I'm not sure that's the most expected one. If the user wants a more detailed format, he can easily set the expected format in the option, so I'm not sure it worth the extra logic.

[benmccann]

I've updated the tests

Code looks good

[simonbrunel]

I'm ok with the code and tests, a bit less with the logic. I would have chosen a simpler path to pick the tooltip format, maybe based on the unit or the tick format (or simply always use the most detailed) but not from the actual data. I don't think there will be big issues with that change but I'm not fan of unstable defaults (e.g. same data displayed in different tz generates different tooltip format).

Not a big deal though, should be fine to merge!?

/cc @etimberg

[etimberg]

@simonbrunel if you're okay with it, let's merge it :)