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

Adding hooks "beforeTooltipDraw" and "afterTooltipDraw" to plugins #4793

Merged
merged 9 commits into from Oct 14, 2017

Conversation

JewelsJLF
Copy link
Contributor

@JewelsJLF JewelsJLF commented Sep 25, 2017

Add a call to plugins.notify before tooltip is drawn. This will allow tooltip to show up on top of a plugin when using "beforeTooltipDraw" inside the plugin.

E.g, Use "beforeTooltipDraw" instead of "afterDraw" in this watermark plugin: https://github.com/JewelsJLF/chartjs-plugin-watermark

@etimberg etimberg added this to the Version 2.8 milestone Sep 29, 2017
@@ -529,6 +529,9 @@ module.exports = function(Chart) {

me.drawDatasets(easingValue);

// notify before drawing tooltips
plugins.notify(me, 'beforeTooltips', [easingValue]);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't beforeTooltipDraw be more explicit and thus durable (e.g. if we decide to add beforeTooltipUpdate or whatever related to the tooltip)? Also, since there is only one managed tooltip, not sure it should be plural.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I'd agree with this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll update it.

…o be more explicit and thus durable (e.g. if we decide to add beforeTooltipUpdate or whatever related to the tooltip)? Also, since there is only one managed tooltip, not sure it should be plural."
@JewelsJLF JewelsJLF changed the title Adding hook "beforeTooltips" to plugins Adding hook "beforeTooltipDraw" to plugins Oct 1, 2017
@@ -529,6 +529,9 @@ module.exports = function(Chart) {

me.drawDatasets(easingValue);

// notify before drawing tooltips
plugins.notify(me, 'beforeTooltipDraw', [easingValue]);
Copy link
Member

Choose a reason for hiding this comment

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

What about making this new hook consistent with *DatasetDraw:

  • make it cancelable: if the hook return false, the tooltip should not be drawn
  • use the new hook signature that accepts only one argument: hook(chart, args, options)

We might want to move that code in a new function, similar to drawDataset:

/**
 * @private
 */
_drawTooltip: function(easingValue) {
    var me = this;
    var tooltip = me.tooltip;
    var args = {
        tooltip: tooltip ,
        easingValue: easingValue
    };

    if (plugins.notify(me, 'beforeTooltipDraw', [args]) === false) {
        return;
    }

    tooltip .draw();

    plugins.notify(me, 'afterTooltipDraw', [args]);
},

Then in draw():

    // ...
    me.drawDatasets(easingValue);
    me._drawTooltip(easingValue);
    
    plugins.notify(me, 'afterDraw', [easingValue]);
    // ...

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

easingValue doesn't seem to be used when drawing the tooltip... any reason to pass it as an arument to the drawTooltip function anyway? Also, any particular reason to call it _drawTooltip instead of just drawTooltip?

Copy link
Member

Choose a reason for hiding this comment

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

easingValue to be consistent with other *Draw hooks (it's more or less the animation progress). We can still add it later on demand if you think it's useless right now. _drawTooltip because it's a private method (there are many other private methods not prefixed because we switched to that convention too late).

…tip" that handles the plugin notifications (beforeTooltipDraw and afterTooltipDraw) and tooltip drawing.
@simonbrunel
Copy link
Member

simonbrunel commented Oct 14, 2017

Could you also update the unit tests with these new hooks (I think it's simply add them before afterDraw)?

* @param {Chart} chart - The chart instance.
* @param {Object} args - The call arguments.
* @param {Object} args.tooltip - The tooltip.
* @param {Object} options - The plugin options.
Copy link
Member

Choose a reason for hiding this comment

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

and move options after args.easingValue :)

@JewelsJLF JewelsJLF changed the title Adding hook "beforeTooltipDraw" to plugins Adding hooks "beforeTooltipDraw" and "afterTooltipDraw" to plugins Oct 14, 2017
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @JewelsJLF

@etimberg I think this PR is safe enough to be included in 2.7.1, what do you think?

@etimberg
Copy link
Member

@simonbrunel I agree. Let's merge this :)

@etimberg etimberg modified the milestones: Version 2.8, Version 2.7.1 Oct 14, 2017
@simonbrunel simonbrunel merged commit 8ac0257 into chartjs:master Oct 14, 2017
@JewelsJLF JewelsJLF deleted the beforeTooltips branch October 14, 2017 22:17
yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants