chartjs / chartjs-plugin-annotation

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

Properly consider the border thickness on the annotations' events #619

Closed stockiNail closed 2 years ago

stockiNail commented 2 years ago

Fix #618

This PR enables the use of the borderWidth in the inRange method for the events.

TODO

stockiNail commented 2 years ago

I think it make sense because we can test not only the positive triggering. At the end, the proposal is to have 1 function for each test case, maybe with more parameters. Or 2 function, 1 for positive and 1 for negative. What do you prefer?

stockiNail commented 2 years ago

maybe the functions could be:

  1. testClickEvents
  2. testMoveEvents

with a positive or negative check (by param).

If you agree, I'd wait for #614 approval/merge because there some common tests which can be moved to events.spec.js, created in the other PR.

stockiNail commented 2 years ago

I'm implementing 3 new functions, 1 for enter, 1 for leave and 1 for click (with dblclick). The other couple of tests (more generic) will be move to a common event spec when the other PR will be merged

stockiNail commented 2 years ago

@kurkle I have committed a temporary implementation in order to, if you have time, you could have a look to event.js and box.spec.js and ellipse.spec.js, if I'm going to the right direction for you.

stockiNail commented 2 years ago

Whe we are sure inRange works correctly, then we can just do one test per event per element (+ features, like label). 4 events, 6 elements, so no more than 50 tests for those.

@kurkle I agree to reduce them. I fact I have seen they are too much.

But I'm a little bit confused because this was at the beginning of this PR.

stockiNail commented 2 years ago

Just for your info, the minimum amount of events test could not be 50 but 100.

Annotation type: 6 Events type: 4 catch/not catch cases: 2 callbacks (common and for each annotation type): 2 Total: 96

EDIT : plus additional tests for specific use cases (i.e. rotation, removed options)

stockiNail commented 2 years ago

@kurkle I would suggest to have another PR for inRange test cases, if you agree

stockiNail commented 2 years ago

@kurkle I would suggest to have another PR for inRange test cases, if you agree

I'm going to add here, in this PR

kurkle commented 2 years ago

@kurkle I would suggest to have another PR for inRange test cases, if you agree

I'm going to add here, in this PR

I'm not sure why. Did I explain why small changes are good?

stockiNail commented 2 years ago

I'm not sure why. Did I explain why small changes are good?

Yes, you did! OK, no problem. I'm still fixing a test case on polygons

stockiNail commented 2 years ago

I have some nits about this still, but I'll create a pr for the nits later, because this is going to take forever if I comment on everything.

ok