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.

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

Conversation

benmccann
Copy link
Contributor

Before

before

After

after

@benmccann benmccann force-pushed the tooltip branch 2 times, most recently from 178a5c2 to 3e1d8f4 Compare July 29, 2017 23:31
@benmccann benmccann closed this Aug 3, 2017
@benmccann benmccann reopened this Sep 19, 2017
@benmccann
Copy link
Contributor Author

@simonbrunel this PR was on hold while you were working on scriptable options. Now that we have scriptable options for the bubble chart would it make sense to also add one for this option on the time scale? I.e. should I just update this PR to take a context parameter or would other changes be required?

@simonbrunel
Copy link
Member

I think so, you should use the options.resolve helper to compute the actual value. The context should be consistent with the one provided by the bubble chart.

@simonbrunel simonbrunel added this to the Version 2.8 milestone Oct 1, 2017
@benmccann benmccann force-pushed the tooltip branch 3 times, most recently from 2009392 to 93fd0e7 Compare October 7, 2017 22:46
@benmccann
Copy link
Contributor Author

@simonbrunel could you give another look to this PR? I don't think I've necessarily done everything as you've intended, in which case I could use some advice, but hopefully it's at least somewhat closer

@benmccann
Copy link
Contributor Author

@simonbrunel thoughts on this one?

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?!

tooltipFormat = typeof tooltipFormat === 'function'
? 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

}

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?

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

@simonbrunel simonbrunel Dec 24, 2017

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.

@simonbrunel
Copy link
Member

@benmccann really sorry for the delay :\

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants