Format the label in the time scale tooltip by benmccann · Pull Request #5095 · chartjs/Chart.js · GitHub

@simonbrunel this is an alternative implementation of #4583

Before

After

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);
}

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

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

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

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?

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.

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

Could be related to old #1466

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

I've added unit tests

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.

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?

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.

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.

@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.

I've updated the tests

Code looks good

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

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

simonbrunel previously requested changes Jan 7, 2018

View reviewed changes

Comment thread src/scales/scale.time.js Outdated

Comment thread src/scales/scale.time.js Outdated

Comment thread src/scales/scale.time.js Outdated

Comment thread src/scales/scale.time.js Outdated

simonbrunel added the type: enhancement label Jan 7, 2018

benmccann force-pushed the tooltip2 branch 5 times, most recently from 280de02 to 9be0d46 Compare January 7, 2018 21:08

simonbrunel reviewed Jan 7, 2018

View reviewed changes

Comment thread src/scales/scale.time.js Outdated

Comment thread src/scales/scale.time.js Outdated

Comment thread src/scales/scale.time.js Outdated

benmccann force-pushed the tooltip2 branch 2 times, most recently from 2998d3e to eb49a75 Compare January 7, 2018 21:35

simonbrunel added this to the Version 2.8 milestone Jan 7, 2018

benmccann force-pushed the tooltip2 branch from eb49a75 to ccc2f5b Compare January 7, 2018 22:41

simonbrunel reviewed Jan 7, 2018

View reviewed changes

Comment thread src/scales/scale.time.js

benmccann force-pushed the tooltip2 branch from ccc2f5b to f070925 Compare January 7, 2018 23:06

simonbrunel requested a review from etimberg January 8, 2018 18:27

benmccann force-pushed the tooltip2 branch from f070925 to ddc1a88 Compare January 9, 2018 03:54

etimberg reviewed Jan 9, 2018

View reviewed changes

benmccann force-pushed the tooltip2 branch 2 times, most recently from 6b16ca5 to 52f9331 Compare January 9, 2018 16:57

simonbrunel reviewed Jan 9, 2018

View reviewed changes

Comment thread test/specs/scale.time.tests.js Outdated

benmccann force-pushed the tooltip2 branch 3 times, most recently from e21e78f to 6c88c38 Compare January 11, 2018 17:50

Format the label in the time scale tooltip

fd770ae

benmccann force-pushed the tooltip2 branch from 6c88c38 to fd770ae Compare January 11, 2018 18:25

etimberg merged commit 37ec838 into chartjs:master Jan 13, 2018

simonbrunel mentioned this pull request Jan 7, 2019

Implement adapter to abstract date/time features #5960

Merged

simonbrunel mentioned this pull request Jan 28, 2019

Time scale performance improvement #6019

Merged

snyk-bot mentioned this pull request Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

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

Format the label in the time scale tooltip (chartjs#5095 )

919460f

+              		if (momentDate.millisecond() !== 0) {
+              			return 'MMM D, YYYY h:mm:ss.SSS a';
+              		}
+              		if (momentDate.second() !== 0 || momentDate.minute() !== 0 || momentDate.hour() !== 0) {

Format the label in the time scale tooltip by benmccann · Pull Request #5095 · chartjs/Chart.js · GitHub

Uh oh!

Format the label in the time scale tooltip - #5095

Format the label in the time scale tooltip#5095etimberg merged 1 commit intochartjs:masterchartjs/Chart.js:masterfrom benmccann:tooltip2benmccann/Chart.js:tooltip2Copy head branch name to clipboard

Conversation

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

Uh oh!

Before

After

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benmccann commented Jan 7, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simonbrunel commented Jan 7, 2018

Uh oh!

benmccann commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonbrunel commented Jan 7, 2018

Uh oh!

simonbrunel commented Jan 7, 2018

Uh oh!

benmccann commented Jan 7, 2018

Uh oh!

Uh oh!

simonbrunel commented Jan 7, 2018

Uh oh!

simonbrunel commented Jan 8, 2018

Uh oh!

benmccann commented Jan 9, 2018

Uh oh!

simonbrunel commented Jan 9, 2018

Uh oh!

etimberg Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

benmccann Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

simonbrunel Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

simonbrunel commented Jan 9, 2018

Uh oh!

benmccann commented Jan 9, 2018

Uh oh!

Uh oh!

simonbrunel commented Jan 12, 2018

Uh oh!

etimberg commented Jan 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Format the label in the time scale tooltip#5095
etimberg merged 1 commit into
chartjs:masterfrom
benmccann:tooltip2

benmccann commented Dec 28, 2017 •
edited
Loading

benmccann commented Jan 7, 2018 •
edited
Loading