search

chartjs / chartjs-plugin-annotation

Annotation plugin for Chart.js
MIT License
607 stars 328 forks source link

Interaction axis options default in the samples is not xy #718

Closed stockiNail closed 2 years ago

stockiNail commented 2 years ago

Default interaction options in the samples are set to 'x' instead of 'xy' as expected.

https://github.com/chartjs/chartjs-plugin-annotation/blob/70078ccbe49568f6a4106667ad008bef7edf5495/docs/scripts/defaults.js#L9-L13

kurkle commented 2 years ago

Why is xy expected?

stockiNail commented 2 years ago

Well, because the samples, where the events are showed, seem to me to be implemented assuming there is axis: xy and not other axis options. Maybe I'm wrong and I can change the title, removing it. The samples was developed where interaction wasn't implemented and then axis was XY and mode nearest even if not declared.

kurkle commented 2 years ago

🤔 I only see one sample that this affects, maybe I'm wrong. But instead, it would be better to fix that 1 sample and also create new samples demonstrating how the different interaction modes can be used in this plugin.

stockiNail commented 2 years ago

Ok, doable. Nevertheless, my opinion as user, I'm expecting to go to the samples and to have the default chart options (in this case interaction is not) because if I'm adding the interaction to the sample, it seems the user should always do. But that's my point of view and maybe I'm wrong. Anyway, let me change the PR following your hint.

kurkle commented 2 years ago

In that case the defaults override should be removed

stockiNail commented 2 years ago

That's the best solution for my perspective. But I don't want to force you to accept it if you have some doubts.

kurkle commented 2 years ago

I'm ok removing the defaults and setting in the samples where required. Its better that way. I think the current defaults are to make the charts work in an opinionated way, which is not good.

stockiNail commented 2 years ago

I'm ok removing the defaults and setting in the samples where required. Its better that way. I think the current defaults are to make the charts work in an opinionated way, which is not good.

Already done

stockiNail commented 2 years ago

About additional sample... to show events, you need a console (I have seen there is already a sample). In my lib showcase, I use checkbox to enable and disable options in order to enable the user to see the different behaviors with different options. Here, correct if I'm wrong, you can use only buttons.

kurkle commented 2 years ago

Yes, at least you need to use a button to trigger an change. I think you could use normal html elements and query their values in the action, but not sure if that is worth the effort.

stockiNail commented 2 years ago

Yes, at least you need to use a button to trigger an change. I think you could use normal html elements and query their values in the action, but not sure if that is worth the effort.

OK, let me close another issue, related to hide some elements methods and then I'll do an event sample