search

chartjs / chartjs-plugin-annotation

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

Change box annotation label as label sub-element #698

Closed stockiNail closed 2 years ago

stockiNail commented 2 years ago

This PR moves label node of box annotation to sub element.

This PR enables the box label animation.

stockiNail commented 2 years ago

@kurkle @LeeLenaleee I have tried to solve all my doubts, listed when this PR was submitted and then now I have removed them.

kurkle commented 2 years ago

Looks like you had an different idea what the sub elements would be, than I did.

My idea was that any annotation could use any other annotation as sub element, by configuring the other element in a certain way. (= DRY)

The idea was not to introduce elements that can only be sub elements of some other element. So instead of "boxLabel" the idea is to have "label" that supports sufficient configuration options to work (also) as a box label. Also the sub element should not need to be aware of the parent element. Parent is the "controller" of the sub element, if you are used to MVC.

Edit: I think the label needs some breaking changes to be used like that. Also the normalization should be done first, to make things easier.

stockiNail commented 2 years ago

Looks like you had an different idea what the sub elements would be, than I did.

Yes, I had a different view on that.

My idea was that any annotation could use any other annotation as sub element, by configuring the other element in a certain way. (= DRY)

The idea was not to introduce elements that can only be sub elements of some other element. So instead of "boxLabel" the idea is to have "label" that supports sufficient configuration options to work (also) as a box label.

Ok even if " supports sufficient configuration options to work (also) as a box label" doesn't seem so easy due to the different options and logic that every label has got. I'll take time to think about, doing some experiments.

Edit: I think the label needs some breaking changes to be used like that. Also the normalization should be done first, to make things easier.

Based on above assumption, yes!

I close it.

stockiNail commented 2 years ago

Change approach as @kurkle suggested

stockiNail commented 2 years ago

@kurkle I know that we should wait for the normalization but I wanted to understand better how sub-elements are working and therefore, following you suggestions, I decided to try with box label (the easiest one). Sorry, but I'm stubborn as a mule. :) This PR will remain in Draft but I wanted to share the code with you in order to understand if this is how you are thinking it should be.

stockiNail commented 2 years ago

Yes, this looks to be more in the direction of my thinking of sub elements.

very good! Anyway this is just a prototype because there are some other items to fix but better to wait for the PR which will address some open issues.

For instance, this PR doesn't take care about the enabled option of box label. Annotation label is using display. #594 must be closed before to do that.

stockiNail commented 2 years ago

@kurkle I have seen another potential annoying thing, going to this direction.

As the issue #589, where we enabled the possibility to change the element.options and invoke the chart.draw to change the rendering of the label.

Going to sub-element, if you change the label option by element instance (i.e. element.options.label), this will not work anymore because the user should change the option of the sub-element (element.elements[0].options). This could be annoying, I guess. It could be good (but I don't think it works) that the options of sub-element is the same object instance of the element.options.