Skip to content

Conversation

@simonbrunel
Copy link
Member

Make all before hooks cancellable (except beforeInit), meaning that if any plugin return explicitly false, the current action is not performed. Ensure that init hooks are called before update hooks and add associated calling order unit tests. Deprecate Chart.PluginBase in favor of IPlugin (no more an inheritable class) and document plugin hooks (also rename extension by hook).

Depends on #3818

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I didn't notice anything major but I still need to run this code

me.clear();

Chart.plugins.notify(me, 'beforeDraw', [easingDecimal]);
plugins.notify(me, 'beforeDraw', [easingDecimal]);
Copy link
Member

Choose a reason for hiding this comment

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

so this can't be cancelled like the other plugins?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

* Plugin extension methods.
* @interface Chart.PluginBase
* Plugin extension hooks.
* @interface IPlugin
Copy link
Member

Choose a reason for hiding this comment

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

We should mention IPlugin#onEvent as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I tested out these changes. I tried all chart types using the sample files and did not notice any issues. I also tested using all of the chartjs-plugin-annotation and chartjs-plugin-zoom samples. No issues were observed.

@etimberg
Copy link
Member

Looks like there is a test failure regarding plugins

@etimberg
Copy link
Member

Will need to change https://github.com/chartjs/Chart.js/blob/master/src/core/core.legend.js#L529 otherwise legend click handling stops working

Make all `before` hooks cancellable (except `beforeInit`), meaning that if any plugin return explicitly `false`, the current action is not performed. Ensure that `init` hooks are called before `update` hooks and add associated calling order unit tests. Deprecate `Chart.PluginBase` in favor of `IPlugin` (no more an inheritable class) and document plugin hooks (also rename `extension` by `hook`).
@etimberg
Copy link
Member

Looks good, the legend is working again :)

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