Enable sub elements #555

kurkle · 2021-12-11T17:08:11Z

requires Chart.js v3.7

stockiNail · 2021-12-11T18:04:04Z

@kurkle what can I say? Cool!
I’m having a look and sounds really good!
Let me know if I can help

kurkle · 2022-01-16T19:08:51Z

Not sure if this should be in v2 or if it would be ok to require a certain minor version of chart.js in an minor update.

kurkle · 2022-01-16T19:09:53Z

Filename	Size	Change
`dist/chartjs-plugin-annotation.esm.js`	12.3 kB	+583 B (+5%)	🔍
`dist/chartjs-plugin-annotation.js`	12.4 kB	+577 B (+5%)	🔍
`dist/chartjs-plugin-annotation.min.js`	8.18 kB	+227 B (+3%)

kurkle · 2022-01-16T19:22:23Z

Makes sense to clip the sub-elements:

to

kurkle · 2022-01-17T13:28:13Z

Not sure if this should be in v2 or if it would be ok to require a certain minor version of chart.js in an minor update.

@etimberg this is the question I requested your review for, but please do a full review too :)

etimberg

Depending on >= 3.7 is fine by me. Is there any way to support other recent versions like 3..5 and 3.6?

src/types/box.js

src/helpers/helpers.core.js

stockiNail · 2022-01-17T21:20:22Z

src/types/box.js

@@ -58,6 +60,7 @@ BoxAnnotation.defaults = {
  cornerRadius: undefined, // TODO: v2 remove support for cornerRadius
  display: true,
  label: {
+    borderWidth: undefined,


I think this new options should be documented. FYI I didn't add borderWidth because I thought that the label in box couldn't have any border (and any background color).
And, maybe I'm wrong, it's never set to the context, therefore is not used to draw the box of the label.

It's actually a leftover abd should be removed. I used this instead of setting the labelOpts.borderWidth = opts.borderWidth, but found out later it was actually the box border width that is used.

👍 do you want to draw a rectangle around the label anyway?

No, I don't have a use case for that right now, but no objection either. I'd ideally like the label to be a sub element, but the label annotation is too fancy for that. Maybe if the callout was a sub element too.. Needs more thought anyway.

I was reviewing this PR and and the usage of sub elements for inner labels (box and line). But you're right, also callout could be.

stockiNail · 2022-01-18T07:28:52Z

src/helpers/helpers.core.js

+      break;
+    }
+    if (isOlderPart(act, req)) {
+      throw new Error(`${pkg} v${ver} is not supported. v${min} or newer is required.`);


@kurkle maybe I'm wrong but this looks like a breaking change.
We are forcing all users to use CHART.JS 3.7.0. Maybe a warning could be emitted and in the version 2.0.0 of the plugin could be an error.
Furthermore, afa I understood, this strong dependency is related to the use of sub elements, currently used only in polygons and then, if a user is not using polygons, is anyway forced to change.

@kurkle if we want to maintain it, we should also add a note in the doc and change the README.

You are probably right, we could do with a warning. The requirement is related to fallback from array options. So applies to use cases that proved annotations as array too. Those "work", but defaults come from line annotation and not from the actual type.

The workaround this PR removed from afterRegister needs to be restored and verify it contains sub-objects of every annotation types defaults.

* Use font.lineHeight instead of font.size for labelling in line annotation (chartjs#475) * Fixes chartjs#474 * removes 1.5 adjustment fo textbaseline set to top * Update src/types/line.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * uses 'middle' instead of 'top' to textBaseline option Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Fix type for CoreAnnotationOptions.borderDash * Bump version to 1.1.0, update deps (chartjs#486) * Update last node version in actions (chartjs#489) * Fix syntax error from previous commit (chartjs#490) * Documentation versioning (chartjs#492) * Documentation versioning * master instead of $VERSION * Add executable bit to docs-config.sh (chartjs#493) * Add executable bit to docs-config.sh * Add docs-config.sh to pull_request * Fixes chartjs#498 (chartjs#500) * Fix broken 'View this example' link (chartjs#499) * Add borderCapStyle and borderJoinStyle options to BoxAnnotation (chartjs#501) * Add borderCapStyle and borderJoinStyle options to BoxAnnotation * removes consistency check on canvas context, too conservative * Add LABEL annotation and label node to BOX annotation (chartjs#502) * Add LABEL annotation and label node to BOX annotation * fixes review and remove inheritance from ellipse annotation * fixes code climate issues * fixes code climate issues (2) * add documentation and sample * adds type definitions * fixes import in options types * re-enables cornerRadius (deprecated) * improves inRange checking the real box of the element * does not export setBorderStyle from helpers because used internally * implements position as object(to manage horizontal and vertical align) * Update test/fixtures/label/scriptableOptions.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Update test/fixtures/box/missingScale.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * fixes review items * adds left and right values for textAlign * fixes content options in box label test case Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Fix line annotation documentation (chartjs#505) * Enable label padding as object (chartjs#506) * Enable label padding as object * adds test cases * adds types * adds documentation * adds test for padding scriptable option invocation * improves the usage of padding instance among functions * re-implements xPadding and yPadding to maintain compatibility * reduces amount of lines of code on applyLabel function * changes content of label keeping them aligned with padding values * Add missing options to types definition (chartjs#517) * Fixes chartjs#516 and chartjs#509 * replaced by the version of master * Add POLYGON annotation (chartjs#503) * Add POLYGON annotation * fixes complexity and max args for a function * adds test cases * adds types * adds documentation * removes polygon sample from "old-samples" folder * Add callout and point nodes to the label annotation (chartjs#507) * Enable callout node in label annotation * adds test cases * fixes code climate * common drawPoint function in helpers * adds documnetation * adds types definitions * adds checks to start option in order to be between 0 and 1 * enables start option as number(px) or string (percentage) * adds check to inRange method if the mouse point is inRange of point * adds test case for events on the point of the callout * adds POINT node to label annotation * reduces Cognitive Complexity to the draw method * adds dblclick test case * removes useless "drawPoint" option from test cases * fixes xScale and yScale options (adding suffix ID) because are wrong. * Throw error when non-existing scale id is used (chartjs#518) * Throw error when non-existing scale id is used * Improve test * Move annotation types handling to types/index.js (chartjs#519) * Log a warning instead of throwing when non-existing scale id is used (chartjs#526) * Revert "Throw error when non-existing scale id is used (chartjs#518)" This reverts commit f2e2481. * Restore Chrome flags * Warn instead * Updates README with new features (chartjs#525) * updates README with new features for 1.2.0 version * changes link to license * Add clip option (chartjs#523) * Add clip option * Add types and test * Add pointStyle and rotation to point options (label and point annotations) (chartjs#521) * adds pointStyle and rotation to point options * adds test cases * adds test cases for label annotation * adds types definitions * adds documentation * fixes source formatting * changes rotation to 180 * replaces by master version * Add test cases to coverage all events cases (chartjs#520) * changes test cases specs adding common events tests * Add test cases * dedicated source file for common events tests * improves objects use in events.js and test case difference percentage * adds additional event test cases and review the display spec * fixes lint issue * adds timeout to simulate 2 clicks instead of dblclick * fixes typo on annotation type for ellipse * Log warning when unknown annotation type is used (chartjs#530) * Run event tests on plugin and annotation levels (chartjs#529) * Use the context of element for events dispatching (chartjs#528) * Use the context of element for dispatching events * Adds test case * Add point positioning by box options for point, polygon and label annotations (chartjs#527) * adds point location by box options (x/yMin and x/yMax) * adds box location to polygon annotation * adds types * adds box positioning for label annotation * fixes test case for label * adds documentation * fixes code climate issue * fixes as suggested by review * change tollerance * fixes review * fixes review * Add note about clip option usage and events catching (chartjs#536) * Fixes chartjs#531 * fixes sentence * Enable HTMLCanvasElement as label content (chartjs#534) * Enable HTMLCanvasElement as label content * adds test case for line annotation * updates types * updates docs * renames isImage to isImageOrCanvas function * Enforce unix linebreaks (chartjs#542) * Enforce unix linebreaks * Fix remaining files * Enable label annotation animations (chartjs#541) * Enable label annotation animations * cc * Split helpers * Add random seed reporting to test runs (chartjs#543) * Resolve circulare dependencies (chartjs#544) * Add samples to the documentation (chartjs#538) * Add samples to the documentation * removes tabs * adds samples to line and box annotations * removes useless assignment * removes CRLF going to LF * temporary commit with checking on CRLF * fixes CRLF * test CRLF avoidance on html files * reverts all files changed for CRLF * adds point samples * adds polygon samples * removes old samples on types folder * adds sample for dataset bars annotation * fixes drawTime * adds label visibility sample * Fix random test failures (chartjs#546) * Disclosure box sample is made responsive (chartjs#548) * Fix `isBoundToPoint` function implementation (chartjs#550) * Fix `isBoundToPoint` function implementation * adds test cases * uses 'defined' function of CHARTJS helpers * Make a consistent implementations of `inRange` in all annotations (chartjs#547) * Enable a consistent implementations of 'inRange' in all annotations * updates line implementation, removing labelRect property * fixes test case because the previous result was wrong (spaceAround) * fixed max number of arguments for a method * some code improvements in inRange method of line annotation * fixes polygon use final position implementation * Add `xAdjust` and `yAdjust` options to point and polygon annotations (chartjs#551) * first commit * adds test cases * adds docs, samples and types * adds adjustment to the samples * renames samples files * additional test case * Fix chartjs#556 (chartjs#558) * Enable label `position` as percentage of the size (chartjs#554) * Fix chartjs#553 * adds box implementation * fixes multiple returns * fixes test case and complexity in box position calculation * reduces duplication code * adds label implementation * changes getSize and toPercent with default * adds documentation * adds types * Update types/label.d.ts Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Update src/types/box.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Update src/types/label.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Update src/types/label.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * applies some review updates * getRelativePosition * temporary commit having a look how to simply the position calculation * rebase to merge PR * fixes double content options in test cases and sample * fixes typo * Update test/fixtures/box/labelPosition.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * adds fixture for position label box Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Fixes chartjs#559 (chartjs#560) * Bump version to 1.2.0, update dependencies (chartjs#561) * Remove old samples (chartjs#564) * remove old samples * remove old samples from the lint * Build docs before releasing as latest/next (chartjs#566) * Remove point node from label annotations in the test cases (chartjs#568) * Publish latest/next docs in separate job (chartjs#567) * Publish latest/next docs in separate job * Missing env * remove if * Add integration tests (ts) (chartjs#569) * Add integration tests (ts) * increase timeout * Increase the integration test timeout to 5 min (chartjs#573) * Workaround for array syntax bug with Chart.js<v3.7 (chartjs#575) * Workaround for array syntax bug with Chart.js<v3.7 * Add a todo * Bump version to 1.2.1 (chartjs#576) * Move scale management functions to a dedicated file (chartjs#574) * Add scale module with all functions to manage the scales * fixes CC * revert CC fixing * sort default options alphabetically (chartjs#570) * Store drawable elements to the state (chartjs#571) * Store drawable elements to the state * reduces complexity of upateElements function * applies review * Fix complexity and duplicated code on scale management (chartjs#578) * Fix complexity and duplicated code on scale management * Update src/scale.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Update src/scale.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * applies review Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Change var to const in the samples (chartjs#584) * Remove undefined event listeners from state if disabled at runtime (chartjs#582) * Removes undefined event listeners from state if disabled at runtime * Update src/events.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Update test/events.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * applies review * revert hint about delete statement Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Fix events catching on rotated ellipse annotations (chartjs#583) * Fix events catching on rotated ellipse annotations * adds specific test for rotated ellipse * new annotation types added to ElementOptionsByType (chartjs#586) Co-authored-by: elias.stocker <elias.stocker@confer.ch> * Fix event test case for rotated ellipse annotation (chartjs#588) * Add borderJoinStyle round to the road signals sample (chartjs#587) * Properly consider the line thickness in the intersect calculation (chartjs#591) * Consider the line thickness in the intersect calculation * fixes similar code * apply review * remove breaking change * Place `performance` commits under Development (chartjs#596) Co-authored-by: Jukka Kurkela <jukka.kurkela@scmbest.fi> * Fix dynamically showing/hiding line label (chartjs#595) * Fix dynamically showing/hiding line label * chore * add test for box * remove borderdash (drawn differently by ff) * inconsistent inRange * Empty label behavior * Revert "Empty label behavior" This reverts commit 32c29c9. * Hide empty labels * cc Co-authored-by: Jukka Kurkela <jukka.kurkela@scmbest.fi> * Bump version to 1.2.2, update deps (chartjs#592) Co-authored-by: Jukka Kurkela <jukka.kurkela@scmbest.fi> * Normalize border style setting (chartjs#598) * Use PI constant of Chartjs helpers instead of Math.PI (chartjs#599) * Fixes events triggering on the label of a line annotation (chartjs#603) * Add JSDoc to `labelIsVisible` method of line annotation (chartjs#604) * Add JSDoc to labelIsVisible method of line annotation * fix lint * apply review * Update src/types/line.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Remove renaming of callback to callHandler (chartjs#605) * Enable shadowing on the annotations (chartjs#600) * adds shadow options to annotations and common functions to set and reset * sets transparent as shadow color default * enables shadowing on point * fixes FF test case tool * adds test cases * adds documentation * adds types * adds samples * applies review * remove a function in favor to a const * changes shadowColor to background/border shadow color * apply review 2 * adds test case on point annotation with borderWidth = 0 * Sort alphabetically the options definitions in the documentation (chartjs#606) * Remove point node from label annotation sample (chartjs#607) * Update labelVisibility sample to demonstrate different approaches (chartjs#609) * Update labelVisibility sample to demonstrate different approaches I wanted to capture some of the discussion from chartjs#589 for other users' benefit. I also removed some properties that were using their default values in order to make the example more focused. * Fix block directives * Sort shadow color default options alphabetically (chartjs#612) * Enable the link to label configuration from the guide navigation menu (chartjs#613) * Enable link to label configuration from the guide navigation menu * fixes typo * Add test cases to test one or both missing scales (chartjs#622) * Add test cases to test one or both missing scales * changes label position in order to recognize the missing scale * applies review * Add specific test cases for ellipse annotation event handling (chartjs#616) * Remove `readValueToProps` function of options helpers (chartjs#617) * Simplify readValueToProps function of options helpers * applies review * Normalize table headers and samples blocks sequence in the documentation (chartjs#623) * Improve event handling adding test cases (chartjs#614) * Add test cases for event handling * adds click callback to test state.moveListened * adds test case for an unmanaged event type * adds test case without any callback but triggering an event * removes if statement because not used * adds test on the state.listened and moveListened * reverts the removing of if statement on event hooks scanning * Fix the scale limit update when annotation value is 0 (chartjs#627) * Fix the default options value in the documentation (chartjs#628) * Remove (some) reduntant properties from fixtures (chartjs#631) * Remove (some) reduntant properties from fixtures * spriteText instead of tolerance * remove "common" * Properly consider the border thickness on the annotations' events (chartjs#619) * Properly consider the border thickness on the annotations' events * adds test cases on label annotation * changes line label inRange calculation * uses testEvents to test events considering border and not * removes decomposition of not used properties * reduces similar code on events test file * refactoring of event test file * applies tests one pixel in the other direction not triggering the hooks * sets borderWidth:0 * adds additional common tests * optimizes the invocation of common code among the specs * adds test cases removing borderWidth at runtime * reduces test cases * refactoring of events test cases * remove comment * fixes ellipse spec test case * fixes annotation type * fixes polygon event test case * adds test cases, removed before cleaning up * removes useless test case because internal * adds test cases for an undefined borderwidth * remove useless options * Remove not used default route on backgroundColor options (chartjs#633) * Box: change event tests to inRange tests (chartjs#632) * Box: change event tests to inRange tests * copy/paste excesss properties * Add arrows decoration to line annotation (chartjs#608) * Add arrowheads options to line annotation * fixes CC 1 * manages borderWidth and the apex of the arrow * adds first 2 test cases * renames function and options and fixes CC * the line is not shown if borderWidth is 0 and more test cases * removes use checks and fixes CC lines of code in the draw method * adds arrowHeadsOptions types * adds documentation * re-applies previous table headers def * apply arrow heads to existing samples * adds test case with scriptable options * fix CC duplicated code * fixes background shadow color for arrow heads and the line * fixes CC max lines per function * applies shadow on border for lines and background on filled arrows * removes useless beginPath * changes display to enabled to be aligned with label subnode * changes display to enabled to be aligned with label subnode - types * apply review about shadow options * enables fallback of arrowHeads.start and arrowHeads.end * apply review 2 * enables borderWidth at arrowHeads level * changes way to get the options to describe for fallback * apply review * fixes label background color in order to work both Chrome and FF * changes defaults in arrowHeads in order to engage the fallback * updates documentation with defaults in fallback * Update src/types/line.js Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * optimized arrowHeads node default Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> * Enable sub elements (chartjs#555) * Enable sub elements * Box chore * helpers chore * CC * Docs, initProperties (for animation staring point) * subElements drawn within clip * Fix clipping + modify fixture to catch * Another fixture * and another fixture * ffs * Box: test borderWidths. Ellipse: change events tests to inRange tests (chartjs#634) * Box: test with different borderWidths * Ellipse: change events tests to inRage tests * Fix auto callout position calculation in label annotation (chartjs#635) * Add test cases for label annotation to test auto callout position * fixes bug related to auto position calculation in the callout * removes useless if statements * if borderWidth of callout is not > 0 the callout is not drawn * adds test cases with wrong position for callout * apply review * Chart.js < v3.7 warning instead of exception (chartjs#638) * Tests: use helper function to fetch elements (chartjs#637) * Remove old test cases (chartjs#639) * Label fixtures: remove point tests, update padding (chartjs#640) * Label fixtures: remove point tests, update padding * create stringify helper * Label fixtures: combine border tests to one file (chartjs#641) * Label fixtures: combine border tests to one file * Add no border and some backgrounds * forgot tolerance * Label fixtures: merge position tests in vertical and horizontal ones (chartjs#643) * Label fixtures: remove position tests with percentage or points * changes max of position vertical chart scale * adds 0%, 100% and invalid position cases * Add tests for missing and empty label content (chartjs#644) * Add label fixture to test missing and empty content * revert * Add label fixture to test invalid and empty content * Label fixtures: merge missing scales test cases (chartjs#645) * Label fixtures: merge missing scales tests * removes useless options * Label fixtures: merge similar callout test cases (chartjs#646) * Label: change event tests to inRange tests (chartjs#648) * Label fixtures: merge canvas test cases and remove useless point test (chartjs#647) * Label fixtures: merge canvas test cases and remove obsolete point test * apply review * Point: change event tests to inRange tests (chartjs#649) * Point: change event tests to inRange tests * apply review * changes location of point 3 * apply review 2 * reverts the context removing * removes context from radius 0 * Polygon: change event tests to inRange tests (chartjs#651) * Polygon: change event tests to inRange tests * fixes polygon inRange checking the radius * adds borderWidth: 0 on some annotations * removes cycles on borderwidth because useless * uses getCenterPoint * removes storing of the vertices * Fix wrong dataset index in sample (chartjs#654) * more fixes * fix defaults * build Co-authored-by: stockiNail <stocki.nail@gmail.com> Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com> Co-authored-by: Marcel <14852157+Marcel0024@users.noreply.github.com> Co-authored-by: Jacco van den Berg <39033624+LeeLenaleee@users.noreply.github.com> Co-authored-by: elitastic <6397763+elitastic@users.noreply.github.com> Co-authored-by: elias.stocker <elias.stocker@confer.ch> Co-authored-by: Jukka Kurkela <jukka.kurkela@scmbest.fi> Co-authored-by: Josh Kelley <joshkel@gmail.com>

kurkle added the enhancement label Dec 11, 2021

kurkle force-pushed the sub-elements branch 2 times, most recently from 8bd10e6 to 97383e3 Compare December 17, 2021 22:15

kurkle force-pushed the sub-elements branch from 97383e3 to 4127d7d Compare December 30, 2021 07:42

kurkle force-pushed the sub-elements branch 4 times, most recently from 9a16a36 to 8e378f1 Compare January 16, 2022 18:20

kurkle marked this pull request as ready for review January 16, 2022 19:06

kurkle requested review from stockiNail and etimberg January 17, 2022 13:25

etimberg reviewed Jan 17, 2022

View reviewed changes

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

src/helpers/helpers.core.js Show resolved Hide resolved

kurkle force-pushed the sub-elements branch from 5b837a2 to 3195893 Compare January 17, 2022 14:30

etimberg previously approved these changes Jan 17, 2022

View reviewed changes

kurkle dismissed etimberg’s stale review via 39fc88d January 17, 2022 14:51

etimberg previously approved these changes Jan 17, 2022

View reviewed changes

kurkle dismissed etimberg’s stale review via 84674b3 January 17, 2022 17:29

kurkle force-pushed the sub-elements branch from 07cd70f to 84674b3 Compare January 17, 2022 17:29

kurkle added 7 commits January 17, 2022 20:05

Enable sub elements

f508958

Box chore

eaeaaca

helpers chore

b8ce7ce

CC

fee14ed

Docs, initProperties (for animation staring point)

eea9f7d

subElements drawn within clip

66097fc

Fix clipping + modify fixture to catch

cf9f648

kurkle added 3 commits January 17, 2022 20:05

Another fixture

c1287af

and another fixture

dd13774

ffs

de506f6

kurkle force-pushed the sub-elements branch from 84674b3 to de506f6 Compare January 17, 2022 18:06

kurkle merged commit 52adba7 into chartjs:master Jan 17, 2022

kurkle deleted the sub-elements branch January 17, 2022 18:14

stockiNail reviewed Jan 17, 2022

View reviewed changes

stockiNail reviewed Jan 18, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable sub elements #555

Enable sub elements #555

kurkle commented Dec 11, 2021 •

edited

stockiNail commented Dec 11, 2021

kurkle commented Jan 16, 2022

kurkle commented Jan 16, 2022

kurkle commented Jan 16, 2022

kurkle commented Jan 17, 2022

etimberg left a comment

stockiNail Jan 17, 2022

kurkle Jan 17, 2022

stockiNail Jan 17, 2022

kurkle Jan 17, 2022

stockiNail Jan 17, 2022

stockiNail Jan 18, 2022

stockiNail Jan 18, 2022

kurkle Jan 18, 2022

Enable sub elements #555

Enable sub elements #555

Conversation

kurkle commented Dec 11, 2021 • edited

stockiNail commented Dec 11, 2021

kurkle commented Jan 16, 2022

kurkle commented Jan 16, 2022

kurkle commented Jan 16, 2022

kurkle commented Jan 17, 2022

etimberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurkle commented Dec 11, 2021 •

edited