Improve tooltips when aggregating data #143
Conversation
| @@ -63,7 +63,7 @@ const timeSeriesChartDefaults = | |||
| time: | |||
| { | |||
| format: 'YYYY-MM-DD', | |||
| tooltipFormat: 'D MMMM YYYY', | |||
| tooltipFormat: 'D MMM YYYY', | |||
There was a problem hiding this comment.
Where is format used? I wonder if the formats should match...
There was a problem hiding this comment.
The format is the default tooltip configuration if you haven’t set a tooltip callback.
Having it spelled out here is mostly a defensive measure now that we have a callback. But I agree that it’s not really necessary anymore. Depends mostly on whether we can reuse that default configuration at a later point in time or not.
|
|
||
| const suffix = (dataPoint.infoText === undefined) | ||
| ? '' | ||
| : ' (' + dataPoint.infoText + ')'; |
There was a problem hiding this comment.
Nit: Wouldn't is be more straight forward to set row['_infoText'] = ''? Then you don't need this condition.
There was a problem hiding this comment.
I thought I’d rather decouple the presentation (putting the info text in parentheses after the title) from the data itself. This keeps the data array rather clean I think even if this conditional is the consequence.
docs/assets/js/charts.js
Outdated
| : ' (' + dataPoint.infoText + ')'; | ||
|
|
||
| if (dateRange === undefined) | ||
| return moment(date).utc().format('D MMM YYYY') + suffix; |
There was a problem hiding this comment.
Would it make sense to define the tooltip format as some kind of global constant? We replicate it 5 times in this PR.
There was a problem hiding this comment.
I also thought about this. However, the interface of Moments.js doesn’t look like this could be extracted.
I would have expected Moments.js to work like this:
moment.utc().format('D MMM YYYY')(1234567890);This would have enabled us to extract the formatting function like this:
const formatDate = moment.utc().format('D MMM YYYY');
formatDate(1234567890);However, as you can see, the interface isn’t like this. Yes, we could move the format into a global configuration, but then there’s still the UTC part that’s replicated everywhere.
There was a problem hiding this comment.
But now thinking of this, let me have a quick look whether there is another interface that would do the trick.
There was a problem hiding this comment.
We could also simply write a wrapper function:
function formatDate(date)
{
return moment(date).utc().format('D MMM YYYY');
}Would you prefer it in this way?
This implements multiple improvements to the tooltips that are shown when hovering data points in views displaying aggregated data. First, the tooltips now show the date range of the period of time over which the data has been aggregated (and not just the first date of the range). An exception to this are the “first” and “last” aggregation methods (which only pick the first or last value from the available time range), where the respective date is shown instead. For reasons of clarity, the date format was changed to show abbreviated month names instead of full ones. Second, the applied aggregation method is shown in parentheses next to the date range. This makes it easier to understand how to interpret the data. Again, this excludes the “first” and “last” aggregation methods, where the date itself already provides all the information needed. Implementation-wise, new auxiliary columns were introduced. These are distinguished from regular columns with a leading underscore character and subsequently ignored from the chart rendering. This mechanism is useful for annotating data, currently with information for showing the tooltips.
This wraps the Moment.js date formatting calls in a separate function to avoid code duplication.
pluehne
force-pushed
the
patrick/show-date-ranges-in-tooltips
branch
from
bc91102
to
1db89be
Compare
|
@larsxschneider: I addressed your feedback concerning code duplication with date formatting in 1db89be. Please have a look at the changes 🙂. |
This implements multiple improvements to the tooltips that are shown when hovering data points in views displaying aggregated data.
First, the tooltips now show the date range of the period of time over which the data has been aggregated (and not just the first date of the range). An exception to this are the
firstandlastaggregation methods (which only pick the first or last value from the available time range), where the respective date is shown instead.For reasons of clarity, the date format was changed to show abbreviated month names instead of full ones.
Second, the applied aggregation method is shown in parentheses next to the date range. This makes it easier to understand how to interpret the data. Again, this excludes the
firstandlastaggregation methods, where the date itself already provides all the information needed.Implementation-wise, new auxiliary columns were introduced. These are distinguished from regular columns with a leading underscore character and subsequently ignored from the chart rendering. This mechanism is useful for annotating data, currently with information for showing the tooltips.
I also updated Moment.js to 2.21.0 because of an issue with how UTC timestamps are handled.
Closes #130.