Fix calculation of dragDistance for click #232
Conversation
|
@benmccann any chance this will be merged any time soon? |
|
I don't have the ability to merge it until someone reviews it |
|
|
||
| if (directionEnabled(chartInstance.$zoom._options.zoom.mode, 'x')) { |
There was a problem hiding this comment.
The previous logic seems better since we avoid calculating extra values in case directionEnabled was false
There was a problem hiding this comment.
The previous logic was buggy, so hardly better :-)
I sent a follow up that's optimized better than the previous version even: #236
I don't think the change will be meaningfully different, but it doesn't hurt either, so I'm not opposed to it
There was a problem hiding this comment.
Obviously, I didn't mean the whole previous logic was better :)
offsetX, startX, endX and dragDistanceX (same for the Y variants) don't need to be calculated if xEnabled (respectively yEnabled) are false (still the case in the new code).
There was a problem hiding this comment.
Hmm. dragDistanceX is used here: https://github.com/chartjs/chartjs-plugin-zoom/blob/master/src/plugin.js#L447. That parameter ends up being ignored in the case that xEnabled=false, but I'm not sure I like the idea of passing in a junk value knowing what that method will do internally. And I'd rather not spend developer time worrying about saving a couple subtraction operations which have no real cost
|
Can you publish a new version on npm that includes this fix? |
|
Thanks again @benmccann for getting this in!!! |
Closes #205