Add point positioning by box options for point, polygon and label annotations

chartjs / chartjs-plugin-annotation

Annotation plugin for Chart.js

MIT License

607 stars 328 forks source link

Add point positioning by box options for point, polygon and label annotations #527

Closed stockiNail closed 2 years ago

stockiNail commented 2 years ago

This PR is adding the feature to locate the point to start drawing a point, a polygon or a label, by box options (xMin, xMax, yMin, yMax).

The radius, is not a number or equals to zero, is calculated by the minimum value between width and height of the box.

Renames the getCircleCenterPoint helpers function to getElementCenterPoint.

Changes parameters to the getRectCenterPoint helpers function. Now it accepts a rect ({x: number, y: number, width: number, height: number}) and not an element and the flag to use the final position anymore.

TODO

[x] box location for point annotation
[x] box location for polygon annotation
[x] box location for label annotation
[x] test cases
[x] types
[x] documentation
[x] waiting for #521 approval and merge

stockiNail commented 2 years ago

~~test/fixtures/label/clip-false.js FAILED coming from another PR. Let me have a look~~

stockiNail commented 2 years ago

~~npm testlog, run locally, exit code 0: https://gist.github.com/stockiNail/2245e56814b1c01233ccce02c63a756f~~ ~~No clear why here it failed~~

stockiNail commented 2 years ago

rerun jobs and everything went well!

stockiNail commented 2 years ago

@kurkle being new as collaborator, having the approval, you gave it, can I merge by myself? Sorry for stupid question. :(

kurkle commented 2 years ago

@kurkle being new as collaborator, having the approval, you gave it, can I merge by myself? Sorry for stupid question. :(

That is not a stupid question. There are number of things to consider before merging.

even if approved, should this be included in next release (=yes in this case)
is anything pending, comments, reviews etc (=no in this case)
consider the order of merging, to minimize the need to rebase (no other pull requests about to be merged currently, so not needed in this case)
its good to wait for any running actions to finish before merging (increases changes of success)

In this case I did not merge it directly, in case you had something to add still. So yes, you can merge it 👍

kurkle commented 2 years ago

Btw, do you have slack? it would be easier to exchange some random thoughts in a chat.

stockiNail commented 2 years ago

Btw, do you have slack? it would be easier to exchange some random thoughts in a chat.

Yes I have it but... I can not use it for this stuff.. It's a long story. Anyway Let me take time to check if and how I can use it.

stockiNail commented 2 years ago

@kurkle being new as collaborator, having the approval, you gave it, can I merge by myself? Sorry for stupid question. :(

That is not a stupid question. There are number of things to consider before merging.
* even if approved, should this be included in next release (=yes in this case)

* is anything pending, comments, reviews etc (=no in this case)

* consider the order of merging, to minimize the need to rebase (no other pull requests about to be merged currently, so not needed in this case)

* its good to wait for any running actions to finish before merging (increases changes of success)
In this case I did not merge it directly, in case you had something to add still. So yes, you can merge it 👍

Thank you very much! They will be more rules for next time!