Using existing ChartJS plugins
niekvanderkooy opened this issue · 31 comments
I'm trying to update Charba from version 2.0 to 2.5.
I was previously using the existing chartjs annotations plugin (https://github.com/chartjs/chartjs-plugin-annotation) as described at the bottom of this page: https://github.com/pepstock-org/Charba/wiki/Plugins,
This worked perfectly in 2.0, but in 2.5 I can no longer get it working. I do not get anny errors, but the plugin just no longer seems to load. See below for the code I have changed. Is there anything I might have forgotten? Also, is the documentation at the bottom of https://github.com/pepstock-org/Charba/wiki/Plugins still correct? Since, for example, Injector.ensureInjected() (without arguments) no longer seems to exist, while the docs still reference it.
For version 2.5, I have changed the following code:
Injector.ensureInjected();
Injector.ensureInjected(Resources.INSTANCE.chartJsAnnotationSource());
to
ResourcesType.setClientBundle(EmbeddedResources.INSTANCE);
Injector.ensureInjected(ResourcesType.getClientBundle().chartJs());
Injector.ensureInjected(ResourcesType.getClientBundle().charbaHelper());
Injector.ensureInjected(Resources.INSTANCE.chartJsAnnotationSource());
Also, I used to include my options as follows:
chart.getOptions().merge(new PlacementTaskAnnotationOptions(initialValue), ChartJsAnnotationOptions.ID);
which I have changed to
chart.getOptions().getPlugins().setOptions(ChartJsAnnotationOptions.ID, new PlacementTaskAnnotationOptions(initialValue));
Finally, the way in which I define properties has been changed, for example from:
private enum Property implements Key {
drawTime,
events,
dblClickSpeed,
annotations
}
to
private enum Property implements Key {
DRAW_TIME("drawTime"),
EVENTS("events"),
DBL_CLICK_SPEED("dblClickSpeed"),
ANNOTATIONS("annotations");
// name value of property
private final String value;
/**
* Creates with the property value to use into native object.
*
* @param value value of property name
*/
private Property(String value) {
this.value = value;
}
/*
* (non-Javadoc)
*
* @see org.pepstock.charba.client.commons.Key#value()
*/
@Override
public String value() {
return value;
}
}
Also, is the documentation at the bottom of https://github.com/pepstock-org/Charba/wiki/Plugins still correct? Since, for example, Injector.ensureInjected() (without arguments) no longer seems to exist, while the docs still reference it.
My fault. I forgot to remove that line Injector.ensureInjected() . I do.
About the injection, the code is correct.
About the plugin config, the code is correct.
About the Property class, yes, we have changed to be aligned with JAVA best practice to use the enumeration items in uppercase (sorry for that), therefore we had to change the interface.
Let me try to recreate a test case with Chart.JS annotation.
Thanks for looking into it!
For reference, these are the complete definitions of my options classes: https://gist.github.com/niekvanderkooy/ba032b3eb966580be4a8f6649552ab1d
@niekvanderkooy first feedback. Even if the documentation of Chart.JS Annotation reports
{
plugins: {
annotation: { ......
}
}
}in the examples they still need the plugin configuration at options level (options as root of plugin configuration node).
Therefore you should try to use merge method, as it was in previous Charba version, as following:
chart.getOptions().merge(new PlacementTaskAnnotationOptions(1L), ChartJsAnnotationOptions.ID);Anyway I'm still investigating because I don't know so in deep the annotation plugin
That is exactly what I am doing (I think. This worked in Charba 2.0, so I think the options that the format of the options is/was correct).
Note that PlacementTaskAnnotationOptions extends ChartJsAnnotationOptions, where ChartJsAnnotationOptions has a property annotations - annotations is not at the root.
Then I create a ChartJsAnnotation annotation = new ChartJsAnnotation();, which I add to the options using this.setAnnotation(annotation).
I tried using merge instead of setOptions, but that still does not work.
@niekvanderkooy got it!
In the class ChartJsAnnotation.java, you set the wrong property names. They have got a COMMA at the end of the name.
private enum Property implements Key {
DRAW_TIME("drawTime,"),
ID("id,"),
TYPE("type,"),
MODE("mode,"),
SCALE_ID("scaleID,"),
VALUE("value,"),
END_VALUE("endValue,"),
BORDER_COLOR("borderColor,"),
BORDER_WIDTH("borderWidth,"),
BORDER_DASH("borderDash,"),
BORDER_DASH_OFFSET("borderDashOffset,"),
LABEL("label,"),
X_SCALE_ID("xScaleID,"),
Y_SCALE_ID("yScaleID,"),
X_MIN("xMin,"),
X_MAX("xMax,"),
Y_MAX("yMax,"),
Y_MIN("yMin,"),
BACKGROUND_COLOR("backgroundColor");Remove them and it should work.
Furthermore, I'd like to open a PR to annotation plugin because they didn't set the plugin ID (they are registering without it and for Chart.js the id is null).
Without this id, the annotation plugin you are enjecting is enable for ALL charts, without exception.
having the ID, you could decide when have the plugin and when not, for every chart where you need.
Let me know if works.
@niekvanderkooy FYI, in the meantime
Added PR #173 to Annotation plugin team to add plugin ID.
Added Issue #174 to Annotation plugin team to change the location where the plugin configuration must be added, to be compliant with CHART.JS best practice.
Wow... Yes, that seems to work. In other words, I made a mistake in my refactor of the Property class!
When I changed that it works on my end as well. Sorry to have wasted your time with this!
By the way, you're right that it only works by using the chart.getOptions().merge() option, not with chart.getOptions().getPlugins().setOptions().
By the way, you're right that it only works by using the chart.getOptions().merge() option, not with chart.getOptions().getPlugins().setOptions().
Yes, I open the issue to plugin team. Let's wait for some feedback from them. When they will update, and you will use that version of plugin, remember to change your code, using getPlugins().setOptions methods.
And apologize again for wiki. I'm gonna change it in next days.
@niekvanderkooy I'd like to use your classes into Charba-Showcase as test case (and example) how to load and use external plugin.
Ok for you?
Yes that's fine! I already anonymized the package name, so you can use whatever you want.
Note that the gist for ChartJsAnnotation.java still contains the wrong property names. Should I leave that, or do you want me to correct it in the gist?
Note that the gist for ChartJsAnnotation.java still contains the wrong property names. Should I leave that, or do you want me to correct it in the gist?
I've copied them in Charba project, changing the package and the property values.
THANK YOU!
@niekvanderkooy FYI, I have seen that Annotation plugin is already able to get configuration from options.plugins, but there is not any published release which contains that update.
I asked to the team if they are going to publish new version or if we have to build it by ourselves, from master. See here
Yeah, I also saw this comment: chartjs/chartjs-plugin-annotation#156 (comment) which indicated that the team is currently not actively working on the plugin. So they might currently not be releasing new versions either :(
yes... honestly I wouldn't take care it...
Of course, we could include it in Charba (maybe rewriting it in GWT instead of JS) but it could take time.
I've got the same issue for chartjs-plugin-labels which seems no longer managed and I don't know if maintain it in Charba or remove it.
Let's wait for any feedback from them
@niekvanderkooy FYI, added new PR to annotation, fixing the build of project.
If you want, here the build of the master with the annotation configuration and plugin id fixed.
@niekvanderkooy just for your info.
Finally we decided to include CHART.JS annotation inside Charba, out of the box, with all capabilities.
Nevertheless, the plugin is not provided by any version (last one is very old) and at every Charba version will document the commit id of the master which will be include.
I don't know if this can help you but we think it could make sense. We are developing it.
@stockiNail How do you mean exactly? That you'll include the plugin as provided by https://github.com/chartjs/chartjs-plugin-annotation directly in chartJS (So there will no longer be a need to provide and inject it manually?)
Or are you implementing a new plugin yourself with the same functionality?
Anyway, sounds good, thanks for keeping me updated!
@niekvanderkooy At the moment we are including the plugin as provided by https://github.com/chartjs/chartjs-plugin-annotation directly in Charba and then you don't have to inject it anymore.
Furthermore you can find all java classes to configure it (out of the box), like annotations, labels and callbacks.
At the beginning this is what we are implementing. Personally I have doubts about the maintainance without a clear version process of the javascript plugin.
If we will see some issues about what I wrote above, we could think to re-implement new plugin (completely in java) with the same capabilities.
Finally what we thought is that a chart libraries could need these kind of capabilities out of the box and delegate the users to implement them could not be a good approach.
This is the reason why we have already included the Zoom plugin as well.
It's clear that we are increasing work to the community but from user stand point this is very confortable.
Make sense?
Ah, ok, yes that's clear!
You're right that this does increase work on Charba, but I agree that features like annotations (and zoom) are probably useful for a lot of users.
I think that, for me, the implementation of ChartJsAnnotation.java, ChartJsAnnotationLabel.java, and ChartJsAnnotationOptions.java was the most work, so especially if these are included in Charba as you suggest, then this would save the user a lot of work.
I think that, for me, the implementation of ChartJsAnnotation.java, ChartJsAnnotationLabel.java, and ChartJsAnnotationOptions.java was the most work, so especially if these are included in Charba as you suggest, then this would save the user a lot of work.
That was we thought. Nevertheless if you will see in our implementation anything could be wrong or not enough comparing with your current implementation, feel free to suggest us how to improve.
I'm gonna reopen the issue that will be closed when we will commit the implementation into master.
We have implemented a first version, finally almost completed (into master).
Nevertheless I'm not 100% convinced about this plugin:
- Box annotation is really poor. It's missing the options to add labels
- Too many events comparing (quite useless) for each annotation,
- Browser warning about the event options set with
passive: true - no version (already mentioned)
I'm gonna spend a couple of days trying to implement a CHARBA plugin with the same capabilities, addressing the above list of items. If it's not complex, I would prefer to have the CHARBA one in order to have full control.
I keep you updated.,
@niekvanderkooy after some further investigation, we are going to release the chart-js-annotation wrapper instead of developing new one.
Ready into master, package annotation
Nevertheless I'm gonna maintain the prototype I did because I'm still conviced that that plugin should be improved.
Let me add in advance that there is an open PR #160 to fix an issue about prevent default events.
I'm gonna provide asap some samples into showcase and write a wiki page (for next version).
Sounds good!
Right now I'm working on a different project, but once I get back to this I will test the built in annotation plugin to see if it all works.
NOP at all, take your time.
In the meanwhile I'm gonna improve it (as much as I can) going to next Charba version. Maybe some classes will change in the next week because I'm trying to fix the plugin defaults options issue which is affecting also this plugin.
@niekvanderkooy maybe can help, in the showcase (even still in my branch) there are HERE some examples how to use the plugin, class names starting with Annotation.
Into Charba master you can find a javascript plugin changed by us in order to avoid an error message when events callback are set.
Nevertheless we are really thinking to rewrite this plugin because it sounds a little unattended.
Just a matter of time
How could you use my help exactly?
I knew you were using the Annotation plugin with your implementation.
Having implemented it out of the box in Charba, maybe you can see anything missing or wrong or different from yours which can be implemented in Charba as well, having a real use case.
I'm pretty sure to have implemented all Annotation capabilities but, getting old, maybe I missed something.
Nevertheless don't waste your time for that. We are adding other use cases with that plugin and I hope we can see any error or missing configuration.
I'm gonna close the issue again.
Thank you very much!
Ah, I understand. Of course I will let you know if I see anything missing, when I start using your out-of-the-box solution for my implementation.
In any case, thanks for all your work.
@stockiNail I've finally had time to try out the integrated Annotations plugin, and it all seems to be working! I've been able to remove my custom integration with the plugin and use the Charba one, so that is nice.
Thanks!
@niekvanderkooy glad to hear that! Very good.
About that plugin, I have the feeling it's a little unattended and more well-maintained.
Let's see what will happen with Chart.js 3.0!
Thank you very much for testing it