search

chartjs / chartjs-chart-financial

Chart.js module for charting financial securities
MIT License
743 stars 199 forks source link

Add hasValue method to elements.Financial #48

Closed nagix closed 5 years ago

nagix commented 5 years ago

Description

When options.tooltips.position is set to 'average' or not specified, the tooltip won't appear. When options.tooltips.position is set to 'nearest', the tooltip will appear but not be centered on the candle.

This happens because the default Element.hasValue checks _model.x and _model.y, but elements.Financial doesn't have _model.y property. This PR adds hasValue method to elements.Financial, which checks _model.x, _model.candleOpen, _model.candleHigh, _model.candleLow and _model.candleClose.

Edit: For tooltipPosition, (vm.candleOpen + vm.candleClose) / 2 is used instead of (vm.candleHigh + vm.candleLow) / 2 so that the tooltip is centered on the fat part of the bar.

Testing

The screenshots below are examples with options.tooltips.position = 'nearest'

Master: https://jsfiddle.net/nagix/ub8m5tzk/

screen shot 2019-02-14 at 12 26 45 am

This PR: https://jsfiddle.net/nagix/dsx4vb6h/

screen shot 2019-02-14 at 12 27 16 am
benmccann commented 5 years ago

Thanks for this!

I wonder also if it'd be nice to update getCenterPoint. It currently does y = (vm.candleHigh + vm.candleLow) / 2;, which centers it between the tips as shown below

screenshot from 2019-02-13 09-45-13

But maybe it'd be better to center it on the fat part of the bar instead? (i.e. use the average of the open and close)

nagix commented 5 years ago

For getCenterPoint, which is used to find nearest element(s), I would keep using (vm.candleHigh + vm.candleLow) / 2. But for tooltipPosition, which gives the tooltip position, I agree that (vm.candleOpen + vm.candleClose) / 2 is better.

benmccann commented 5 years ago

Thanks again @nagix ! I'm curious what led you to try out this chart. Are you using it for anything?

nagix commented 5 years ago

I have integrated chartjs-plugin-streaming with chartjs-chart-finance (demo), and there was an issue report today (nagix/chartjs-plugin-streaming#65).

benmccann commented 5 years ago

That's great!

benmccann commented 5 years ago

Btw, I noticed the first couple seconds of the demo things move around a lot. It might be nice to start the chart with some bars already present instead of starting with an empty chart. In any case, I think this is a very cool demo and it's awesome to see!

nagix commented 5 years ago

Thanks for your feedback🙂 I will update it in the next commit.