Skip to content
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.

Sign up for GitHub

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

Allow tooltipFormat to be a function. Provide a default tooltipFormat #4583

Closed
wants to merge 1 commit into from
Closed

Allow tooltipFormat to be a function. Provide a default tooltipFormat #4583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions docs/axes/cartesian/time.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ The following options are provided by the time scale. You may also set options p
| `time.min` | [Time](#date-formats) | | If defined, this will override the data minimum
| `time.parser` | `String/Function` | | Custom parser for dates. [more...](#parser)
| `time.round` | `String` | `false` | If defined, dates will be rounded to the start of this unit. See [Time Units](#time-units) below for the allowed units.
| `time.tooltipFormat` | `String` | | The moment js format string to use for the tooltip.
| `time.tooltipFormat` | `String/Function` | | The moment js format string to use for the tooltip. [more...](#tooltip-format)
| `time.unit` | `String` | `false` | If defined, will force the unit to be a certain type. See [Time Units](#time-units) section below for details.
| `time.stepSize` | `Number` | `1` | The number of units between grid lines.
| `time.minUnit` | `String` | `'millisecond'` | The minimum display format to be used for a time unit.
Expand Down Expand Up @@ -147,6 +147,11 @@ The `ticks.source` property controls the ticks generation
* `'labels'`: generates ticks from user given `data.labels` values ONLY

### Parser
If this property is defined as a string, it is interpreted as a custom format to be used by moment to parse the date.
If this property is defined as a string, it is interpreted as a custom format to be used by Moment.js to parse the date.

If this is a function, it must return a moment.js object given the appropriate data value.
If this is a function, it must return a Moment.js object given the appropriate data value.

### Tooltip Format
If this property is defined as a string, it is interpreted as a custom format to be used by Moment.js to format the date.

If this is a function, it must return a custom format to be used by Moment.js to format the date.
49 changes: 41 additions & 8 deletions src/scales/scale.time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,11 @@ function parse(input, scale) {
/**
* Returns the number of unit to skip to be able to display up to `capacity` number of ticks
* in `unit` for the given `min` / `max` range and respecting the interval steps constraints.
* @param min {number} minimum tick millis
* @param max {number} maximum tick millis
* @param unit {string} the time unit the ticks are being displayed as
* @param capacity {number} the number of labels we have room to display
* @return {string} the number of ticks to skip before displaying the next one
*/
function determineStepSize(min, max, unit, capacity) {
var range = max - min;
Expand Down Expand Up @@ -403,6 +408,21 @@ function ticksFromTimestamps(values, majorUnit) {
return ticks;
}

/**
* Show the most specific time format by default
*/
function defaultTooltipFormat(context) {
Copy link
Member

@simonbrunel simonbrunel Dec 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand it's the default you want for the financial chart but I'm not sure about using it as default for all charts. I usually prefer consistency when reading data and IMO having the tooltip display format changing for all values is not a good user experience. I would go for a static default (maybe the most detailed 'MMM D, YYYY h:mm:ss.SSS a' - see comment below) - this would also avoid converting, by default, 2 times the same value to a Moment object (though that's minor).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I slept on it and finally, I think the best default behavior is to keep tooltipFormat undefined and handle this case in getLabelForIndex as: if tooltipFormat is null or undef, use the same display format as the one chosen for the scale labels, so by default, the tooltip displays exactly the same text as the tick label (I remember a ticket or comment requesting that behavior).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of what I was trying to accomplish here was to show more detail in the tooltip than is on the scale as that's a common use of tooltips that I think people expect. E.g. if the scale says "May 11" then it would be nice that when you mouseover with the tooltip that you see "May 11, 2017". I think using the same display format as the one chosen for the scale labels is not a bad choice either. Perhaps we could have some way to toggle between these two behaviors? E.g. define the tooltipFormat as this function, but then if you set it to null then it displays the same as the scale or something like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My issue with that method is that the tooltip displays different formats depending on the current value, which I think it's not a good UX. It can be different from the tick label (more detailed) but should be the same for all the values, so when the user moves his mouse, he always reads the same format.

That's why I would implement the way I described in my second comment since it's simpler, more optimized and I think more expectable as a default.

Though that would be great to have more feedback.

var value = context.dataset.data[context.dataIndex];
var momentDate = momentify(value, context.options.time);
if (momentDate.millisecond() !== 0) {
return 'MMM D, YYYY h:mm:ss.SSS a';
}
if (momentDate.second() !== 0 || momentDate.minute() !== 0 || momentDate.hour() !== 0) {
return 'MMM D, YYYY h:mm:ss a';
}
return 'MMM D, YYYY';
}

module.exports = function(Chart) {

var defaultConfig = {
Expand Down Expand Up @@ -434,6 +454,7 @@ module.exports = function(Chart) {
displayFormat: false, // DEPRECATED
isoWeekday: false, // override week start day - see http://momentjs.com/docs/#/get-set/iso-weekday/
minUnit: 'millisecond',
tooltipFormat: defaultTooltipFormat,

// defaults to unit's corresponding unitFormat below or override using pattern string from http://momentjs.com/docs/#/displaying/format/
displayFormats: {
Expand Down Expand Up @@ -626,19 +647,31 @@ module.exports = function(Chart) {

getLabelForIndex: function(index, datasetIndex) {
var me = this;
var data = me.chart.data;
var timeOpts = me.options.time;
var chart = me.chart;
var data = chart.data;
var options = me.options;
var timeOpts = options.time;
var tooltipFormat = timeOpts.tooltipFormat;
var label = data.labels && index < data.labels.length ? data.labels[index] : '';
var value = data.datasets[datasetIndex].data[index];
var context = {
chart: chart,
options: options,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not add options as part of context because it's confusing: could be the chart options or the scale options. Maybe scale (this) is better if we already expose the scale object publicly somewhere else?!

dataIndex: index,
dataset: chart.data.datasets[datasetIndex],
datasetIndex: datasetIndex
};
var momentDate;

if (helpers.isObject(value)) {
label = me.getRightValue(value);
}
if (timeOpts.tooltipFormat) {
label = momentify(label, timeOpts).format(timeOpts.tooltipFormat);
if (!helpers.isObject(value)) {
return label;
}

return label;
momentDate = momentify(me.getRightValue(value), timeOpts);
tooltipFormat = typeof tooltipFormat === 'function'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use helpers.options.resolve() here?

? tooltipFormat.call(me, context)
: tooltipFormat;
return momentDate.format(tooltipFormat);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to handle null/undefined tooltipFormat since it's currently supported

},

/**
Expand Down
1 change: 1 addition & 0 deletions test/specs/scale.time.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ describe('Time scale tests', function() {
isoWeekday: false,
displayFormat: false,
minUnit: 'millisecond',
tooltipFormat: defaultConfig.time.tooltipFormat,
displayFormats: {
millisecond: 'h:mm:ss.SSS a', // 11:20:01.123 AM
second: 'h:mm:ss a', // 11:20:01 AM
Expand Down