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

WIP: Scriptable tooltip options #5981

Closed
wants to merge 6 commits into from

Conversation

etimberg
Copy link
Member

@etimberg etimberg commented Jan 13, 2019

This is a WIP version of scriptable options for tooltips.

To Do

  • Improve getBaseModel function for better minification
  • Make body options scriptable per line of body text
  • Tests
  • Documentation

@etimberg
Copy link
Member Author

@simonbrunel @kurkle @nagix @benmccann I'd love to get your thoughts on this and whether or not you think it's moving in the right direction. I didn't want to dive too far into it without some discussion.

@simonbrunel
Copy link
Member

simonbrunel commented Jan 13, 2019

When I introduced scriptable options, the idea was to keep the "context" generic, consistent, minimal and maintainable, while providing a way for users to implement whatever use cases. The scriptable context should not be designed for a specific feature and should be interchangeable (e.g. using the same function to control the color of the bar, the box in the legend or in the tooltip).

That's why I think exposing tooltipItems doesn't go in the right direction because it only applies to the tooltip and will be a pain to maintain (the tooltip items API is too complex to be part of the context). We already exposed too much in the tooltip callbacks, we should be careful with scriptable options :)

#5980 is one use case and we shouldn't design scriptable options for that specific request. Your suggestion of using scriptables for the tooltip title kinda makes sense (even if there is no "loop" - thus, no different context - for the tooltip title). The context could be context.chart only and users would access the context.chart.tooltip.* public API to implement whatever he wants. If there is no method to retrieve the current tooltip items, I would rather add one (e.g. tooltip.items()) than specializing the context (I would do that in a separate PR).

Make body options scriptable per line of body text

This makes sense because we can generate the same context as the controller options (chart, dataset, dataIndex, datasetIndex).

@etimberg
Copy link
Member Author

I agree that we should keep the context simple. I'm also OK to wrap the tooltipItems array as a getter on the tooltip itself. In v3 we could look at porting the sort & filter methods as well so that we don't have to provide direct options for them.

The tooltipItems themselves look like they could become the context at some point since they already have information on the dataIndex and datasetIndex.

@etimberg
Copy link
Member Author

@simonbrunel I improved the minification & removed the tooltip items from the context. This still needs tests and documentation. Thoughts so far?

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

I did not go through all the options and fallback, but overall looks good to me.

src/core/core.tooltip.js Outdated Show resolved Hide resolved
var data = me._data;
var i, len;
Copy link
Contributor

Choose a reason for hiding this comment

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

we usually put unitialized variables after all initialized variables, so I might move this back to where it was originally

src/core/core.tooltip.js Outdated Show resolved Hide resolved
@etimberg etimberg force-pushed the scriptable-tooltip-options branch from 774f0e9 to e486419 Compare July 15, 2019 21:25
@benmccann
Copy link
Contributor

I remember there being a report that tooltip can be slow on a chart with lots of data points. Is update called only when the chart is updated or is it called each time a point is hovered? If the latter, then I'm wondering what the performance impact of this change would be

@etimberg etimberg closed this Oct 27, 2019
@etimberg etimberg deleted the scriptable-tooltip-options branch October 27, 2019 20:45
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.

4 participants