Plugins Enhancement #2752
Merged
Plugins Enhancement #2752
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rename `Chart.pluginService` to `Chart.plugins` (so move the old Chart.plugins array as a private member of the service), and rename `notifyPlugins` to `notify` for consistency with other service methods.
Change the plugin notification behavior: this method now returns false as soon as a plugin *explicitly* returns false, else returns true. Also, plugins are now called in their own scope (so remove the never used `scope` parameter).
Member
Author
simonbrunel
force-pushed
the
plugins-arch
branch
from
0484180 to
b662d0c
Compare
The plugins service now accepts an array of plugin instances to register or unregister (for consistency, renamed `Chart.plugins.remove` to `unregister`). Also added a few methods to manipulate registered plugins, such as `count`, `getAll` and `clear` (mainly used by our unit tests).
Add `beforeDatasetsUpdate` and `afterDatasetsUpdate` plugin notifications during the chart update. Plugins are able to cancel the datasets update by explicitly returning false to `beforeDatasetsUpdate`. For consistency, rename `(before|after)DatasetDraw` to `(before|after)DatasetsDraw`.
simonbrunel
force-pushed
the
plugins-arch
branch
from
b662d0c to
53eb766
Compare
| * Unregisters the given plugin(s) only if registered. | ||
| * @param {Array|Object} plugins plugin instance(s). | ||
| */ | ||
| unregister: function(plugins) { |
Member
There was a problem hiding this comment.
for completeness we should implement remove to alias to unregister
Member
Author
There was a problem hiding this comment.
I thought the same first but then realized that this method has never been used in code or plugins (correct me if I'm wrong), and I can't see any case where it should be used (except in unit tests). So was thinking that we may not keep compatibility and save a few bits. What do you think?
Member
There was a problem hiding this comment.
Sure, I don't think anyone has used it.
Member
|
Other than my 1 comment this is good to go Edit: did not mean to close, sorry |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mostly explained in commit messages, but on the menu:
Chart.pluginServicetoChart.plugins,Chart.pluginService.removetoChart.plugins.unregister,Chart.pluginService.notifyPluginstoChart.plugins.notify,falseas soon as a plugin explicitly returnsfalse(can be used, for example, to cancel an action),beforeDatasetsUpdate(cancellable) andafterDatasetsUpdate,before/afterDatasetDrawtobefore/afterDatasetsDraw(with s to Datasets),Note: I need these changes to implement a plugin to address #2745