Rewrote the annotation update handler, bugfixes#45
Conversation
0a3524a to
fbb5c84
Compare
fbb5c84 to
b5cf142
Compare
etimberg
left a comment
There was a problem hiding this comment.
A few minor comments. I haven't had a chance to run this yet. I will try and do that tomorrow
| var element; | ||
| if (typeof annotation._id === 'undefined' && !!annotationTypes[annotation.type]) { | ||
| annotation._id = helpers.objectId(); | ||
| var cls = annotationTypes[annotation.type]; |
There was a problem hiding this comment.
not sure if a linter would complain about the first character not being a capital
There was a problem hiding this comment.
jshint doesn't complain. I'd rather have it lowercase, because convention is that newable classes are capital. cls is a variable class reference, not the class definition itself. I think this subtle difference makes the code more readily understandable.
| ns.config.annotations.forEach(function(annotation) { | ||
| var element; | ||
| if (typeof annotation._id === 'undefined' && !!annotationTypes[annotation.type]) { | ||
| annotation._id = helpers.objectId(); |
There was a problem hiding this comment.
do you think it would be worth it to make this property public so that users could define their own IDs for annotations?
There was a problem hiding this comment.
That's an interesting idea. I wouldn't want to be required to define it, and while I can't think of any reason why I'd want to set a specific ID, it's better to err on the side of flexibility. It would be a simple change here.
| // Configure plugin namespace | ||
| Chart.Annotation = Chart.Annotation || {}; | ||
|
|
||
| Chart.Annotation.dblClickSpeed = 350; // ms |
There was a problem hiding this comment.
did we document this property in the readme?
|
|
||
| var helpers = require('./helpers.js')(Chart); | ||
|
|
||
| Chart.Annotation.ScaleAdjuster = function(scale) { |
There was a problem hiding this comment.
Is the intent of putting this at Chart.Annotation.ScaleAdjuster to allow it to be replaced easily by a consumer of this lib?
There was a problem hiding this comment.
Not replaced necessarily (although that would be possible), but used or reused in other places. Originally did it this way because I thought that I would have to have users manually add it to their scales options objects, but then I found a way to add it automatically at the right time, so this doesn't need to be public anymore.
etimberg
left a comment
There was a problem hiding this comment.
Looks good. I tested out the behaviour and didn't notice any problems.
I think the package.json needs to be updated to specify Chart.js v2.4.0 as a dependency. The distanceBetweenPoints helper was introduced then.
* Fixes a bug where calling `chartInstance.update()` broke the referential integrity of the annotations, preventing further updates * Fixes a bug where the `hovering` element flag was getting reset when calling `chartInstance.update()` from inside the `onMouseover` handler * Annotations now use an unambiguous unique ID rather than an array index to map configs to element instances * Properly hooked in the scale adjuster using the `afterDataLimits` scale option * Refactored the plugin object to its own file * Refactored helpers.js to use a consistent module format * Pointed the `main` property in `package.json` back to the `src` directory to deal with Webpack warnings * Tighter plugin code and var names
b5cf142 to
4bb9f92
Compare
Includes chartjs#44, chartjs#45
chartInstance.update()broke the referential integrity of the annotations, preventing further updateshoveringelement flag was getting reset when callingchartInstance.update()from inside theonMouseoverhandlerafterDataLimitsscale optionmainproperty inpackage.jsonback to thesrcdirectory to deal with Webpack warnings