Normalize annotations elements to be based on common box model #706

stockiNail · 2022-03-01T14:28:17Z

This PR normalizes the annotations elements to the following box model:

stockiNail · 2022-03-01T15:45:48Z

@kurkle for other annotations, where the changes could depend on these ones, of this PR, do you prefer other PRs (I was thinking 1 for point and polygon and 1 for line) or to apply the changes all in this ones? Of course, the second one is faster, no dependency.

src/types/base.js

kurkle · 2022-03-04T17:36:48Z

@kurkle for other annotations, where the changes could depend on these ones, of this PR, do you prefer other PRs (I was thinking 1 for point and polygon and 1 for line) or to apply the changes all in this ones? Of course, the second one is faster, no dependency.

Probably best to do all at once, in this case.

stockiNail · 2022-03-07T07:36:23Z

Probably best to do all at once, in this case.

I'm gonna work on that this week!

stockiNail · 2022-03-09T09:56:25Z

@kurkle offtopic. After new Karma version, when I run npm run dev (also dev:ff), the index.js file of tests is changed with a source code completely different (auto generated).

[]\chartjs-plugin-annotation>npm run dev:ff

> [email protected] dev:ff
> karma start --auto-watch --no-single-run --browsers firefox

INFO [preprocessor.rollup]: Generating bundle for ./src\index.js
INFO [preprocessor.rollup]: Generating bundle for ./test\index.js
WARN [karma]: No captured browser, open http://localhost:9876/
INFO [karma-server]: Karma v6.3.17 server started at http://localhost:9876/
INFO [launcher]: Launching browsers firefox with concurrency unlimited
INFO [launcher]: Starting browser Firefox
INFO [preprocessor.rollup]: Generating bundle for ./test\index.js
INFO [filelist]: Changed file "[]/chartjs-plugin-annotation/test/index.js".  <----- HERE
WARN [karma]: No captured browser, open http://localhost:9876/
INFO [Firefox 97.0 (Windows 10)]: Connected on socket IxXwYXD3S64JKcl2AAAB with id 52745204
Firefox 97.0 (Windows 10) WARN: 'Testing with chart.js v3.7.1'

Do you have the same issue and behavior or it's something related to my env?

kurkle · 2022-03-09T10:01:20Z

Do you have the same issue and behavior or it's something related to my env?

Yes, its karma-rollup-preprocessor 7.0.8 that is causing it. Fix: npm i [email protected] --save-dev

Ref: jlmakes/karma-rollup-preprocessor#75

stockiNail · 2022-03-09T10:56:26Z

Yes, its karma-rollup-preprocessor 7.0.8 that is causing it. Fix: npm i [email protected] --save-dev

Shall I commit the update of package-lock.json?

Conflicts: src/helpers/helpers.chart.js src/helpers/helpers.core.js src/types/box.js src/types/ellipse.js src/types/label.js src/types/line.js src/types/polygon.js

docs/guide/migrationV2.md

stockiNail · 2022-04-06T13:46:50Z

@LeeLenaleee I have added a drawio file for each annotation type (new diagrams folder in doc) and exported them in PNG in img folder. Let me know if you think it could be improved from your point of view.

kurkle · 2022-04-06T14:32:55Z

src/helpers/helpers.chart.js

@@ -95,9 +95,9 @@ export function getChartPoint(chart, options) {
 /**
 * @param {Chart} chart
 * @param {CoreAnnotationOptions} options
- * @returns {{x?:number, y?: number, x2?: number, y2?: number, width?: number, height?: number}}
+ * @returns {{x:number, y: number, x2: number, y2: number, centerX: number, centerY: number, width: number, height: number}}


I think the return value deserves its own type, maybe called AnnotationBoxModel?

Ok, makes sense

We have already AnnotationElement even if it includes options in its definition. in my opinion, we need to map the new model in more TS types.

kurkle · 2022-04-06T14:37:41Z

src/helpers/helpers.core.js

@@ -30,8 +30,8 @@ export function inBoxRange(point, {x, y, width, height}, axis, borderWidth) {
 }

 export function getElementCenterPoint(element, useFinalPosition) {
-  const {x, y} = element.getProps(['x', 'y'], useFinalPosition);
-  return {x, y};
+  const {centerX, centerY} = element.getProps(['centerX', 'centerY'], useFinalPosition);


Would it be better for backward compatibility, if we used x and y for the center point? (and x1,y1 for the top/left)

On the other hand centerX and centerY are explicit. Could also go further explicit and use top, left, bottom, right for the corner coordinates.

Would it be better for backward compatibility, if we used x and y for the center point? (and x1,y1 for the top/left)

make sense!

On the other hand centerX and centerY are explicit. Could also go further explicit and use top, left, bottom, right for the corner coordinates.

Could make sense but in this way we don't have any backward compatibility, for any annotation type. Do you want I do in this PR or with another one?

@kurkle be also aware that top, left, bottom and right can not map very well the line annotation because bottom could be greater than top.

Would it be better for backward compatibility, if we used x and y for the center point? (and x1,y1 for the top/left)

make sense!

@kurkle sorry but I think I misunderstood your comment. In my poor option it couldn't be a good idea to use x,y for center because it's not readable, I guess. The getCenterPoint method returns {x: number, y: number}.

Not sure what you mean with "not readable"? The element properties can be read directly from the element. But if you mean its not obvious what x and y are, I agree.

I think its ok to go with centerX/Y, just wanted to bounce these thoughts.

Not sure what you mean with "not readable"? The element properties can be read directly from the element. But if you mean its not obvious what x and y are, I agree.

I meant "user friendly" or obvious (as you wrote). I didn't the user can not access to them ;)

@kurkle be also aware that top, left, bottom and right can not map very well the line annotation because bottom could be greater than top.

@kurkle I thought about line annotation. We could go to top, left, bottom and right model but we need to add some additional logic to know which corners of the box must be join to draw the line to the right direction.
Let me know what I think it's better.

stockiNail · 2022-04-06T20:02:08Z

@kurkle I have seen your approval.
Does it mean you dont want to evaluate your proposed model? Or just postponed to another PR?

stockiNail · 2022-04-06T20:19:58Z

If you agree I would like to spend a couple of days, maybe next week, in a PR where top left bottom right model could be used.
Personally I think could make sense more than x x2 y y2, that I dont love it. As said the line annotation could need more time and thoughts.

kurkle · 2022-04-07T07:18:02Z

If you agree I would like to spend a couple of days, maybe next week, in a PR where top left bottom right model could be used. Personally I think could make sense more than x x2 y y2, that I dont love it. As said the line annotation could need more time and thoughts.

Sure go ahead and explore!

stockiNail · 2022-04-07T07:37:04Z

@kurkle about AnnotationBoxModel and JSDoc, I thought yesterday and I'll submit a PR to add JSDOC to all exported functions and public methods. Doing that, I'll review also the types to add to JSDOC. I was thinking to add JSDOC to the types (currently there are some jsdoc but not to all, therefore we bothadd to or remove form all) but I hand over the decision to you, if make sense.

stockiNail added 2 commits March 1, 2022 09:05

changed box annotation

5e7697f

apply new box model on label, box and ellipse annotations and tests

798ba76

stockiNail added breaking change enhancement labels Mar 1, 2022

stockiNail added this to the 2.0.0 milestone Mar 1, 2022

removes useless "this" reference

2203073

stockiNail removed the enhancement label Mar 2, 2022

kurkle reviewed Mar 4, 2022

View reviewed changes

src/types/base.js Outdated Show resolved Hide resolved

stockiNail added 5 commits March 8, 2022 15:47

changes point and polygon annotations

caec490

fixes CC

df46aec

removes getRectCenterPoint function

8344dd6

move back getChartCircle function and renames resolve functions

81891e0

Merge remote-tracking branch 'origin/master' into normalizeBox

28168ad

apply model to line annotation

9665724

stockiNail changed the title ~~Normalize box, ellipse and label annotations to be based on common box model~~ Normalize annotations elements to be based on common box model Mar 9, 2022

stockiNail added 3 commits March 9, 2022 11:21

changes types

57c162b

re-use inBoxrange function in the line label

1d6446a

improves translate function

223414f

stockiNail added 4 commits March 9, 2022 15:45

adds center* properties to animation

869bc05

Merge remote-tracking branch 'origin/master' into normalizeBox

a4004ad

Conflicts: src/helpers/helpers.chart.js src/helpers/helpers.core.js src/types/box.js src/types/ellipse.js src/types/label.js src/types/line.js src/types/polygon.js

changes default animation numbers properties

c66d682

adds note about breaking change to migration guide

87daf8c

LeeLenaleee reviewed Apr 6, 2022

View reviewed changes

docs/guide/migrationV2.md Outdated Show resolved Hide resolved

fixes CC

e9db75b

removes partial sentence from migration guide

dd3f731

stockiNail marked this pull request as ready for review April 6, 2022 11:43

stockiNail requested review from kurkle and LeeLenaleee April 6, 2022 11:48

adds diagrams about new element properties for each annotation type

5cd9c0a

kurkle reviewed Apr 6, 2022

View reviewed changes

kurkle approved these changes Apr 6, 2022

View reviewed changes

stockiNail merged commit 43002ff into chartjs:master Apr 7, 2022

stockiNail deleted the normalizeBox branch April 7, 2022 07:37

stockiNail mentioned this pull request Apr 8, 2022

Add JSDoc to exported functions #722

Merged

Normalize annotations elements to be based on common box model #706

Normalize annotations elements to be based on common box model #706

Uh oh!

Conversation

stockiNail commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stockiNail commented Mar 1, 2022

Uh oh!

Uh oh!

kurkle commented Mar 4, 2022

Uh oh!

stockiNail commented Mar 7, 2022

Uh oh!

stockiNail commented Mar 9, 2022

Uh oh!

kurkle commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stockiNail commented Mar 9, 2022

Uh oh!

Uh oh!

stockiNail commented Apr 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stockiNail commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stockiNail commented Apr 6, 2022

Uh oh!

kurkle commented Apr 7, 2022

Uh oh!

stockiNail commented Apr 7, 2022

Uh oh!

Uh oh!

stockiNail commented Mar 1, 2022 •

edited

Loading

kurkle commented Mar 9, 2022 •

edited

Loading

stockiNail commented Apr 6, 2022 •

edited

Loading