search

chartjs / chartjs-plugin-annotation

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

Make a consistent implementations of `inRange` in all annotations #547

Closed stockiNail closed 2 years ago

stockiNail commented 2 years ago

Fix #535

stockiNail commented 2 years ago

@kurkle see the polygon implementation. I'm not convinced at 100%

stockiNail commented 2 years ago

@kurkle I'm working on this PR but I need time... I'm fighting with the updates on line annotations

stockiNail commented 2 years ago

@kurkle I think I found a bug, updating the line.

https://github.com/chartjs/chartjs-plugin-annotation/blob/e78e1cfb53ffb939aba849739585d94b1f06806c/src/types/line.js#L323-L324

When the top and bottom are equals, -1 is returned. The same for left and right.

image

In my opinion, the check should be <= and not only =.

image

stockiNail commented 2 years ago

I have fixed it in this PR.

stockiNail commented 2 years ago

I think it would be really good to have sub elements. Like point for polygon, and label. Almost same as having the labelRect, but it would be a constructed class handling its own coordinates and options. And updated in similar way the main annotation elements are updated. That would reduce complexity of the elements, enable animation with simple x,y,width,height properties etc. There are probably some cons too.

Just a brain dump here, I will try something out when I have couple of hours to spare.

Ok, now it's clear. But I think this new classes (for instance Label) should extend Element and must be registered as well, doesn't it?

For sure some cons are related that some behaviors (about the label mainly) are different between the annotations.

stockiNail commented 2 years ago

Just a brain dump here, I will try something out when I have couple of hours to spare.

I'm curious and I want to try as well, by myself, in a separated branch! I think it makes sense because this morning I was thinking about the huge possibilities to enrich the plugin with the animations.

kurkle commented 2 years ago

There again I hit the Chart.js resolver bug that is fixed only in current master (will be out in Chart.js v3.7)