demo/stock-orderbook

[kamil-musialowski]

Created a live demo simulating stock orderbook charts.

[highsoft-bot]

Webpack monitoring

LGTM 👍

[highsoft-bot]

File size comparison

No differences found

[highsoft-bot]

Visual test results - No difference found

---

Samples changed

Change type	Sample
Modified	samples/stock/demo/orderbook-chart/demo.js
Modified	samples/highcharts/studies/map-activate/demo.js
Modified	samples/highcharts/studies/map-activate/demo.js
Modified	samples/stock/demo/orderbook-chart/demo.js
Modified	samples/stock/demo/orderbook-chart/demo.js
Modified	samples/stock/demo/orderbook-chart/demo.js
Modified	samples/stock/demo/orderbook-chart/demo.js
Modified	samples/stock/demo/orderbook-chart/demo.js
Modified	samples/stock/demo/orderbook-chart/demo.js
Modified	samples/stock/demo/orderbook-chart/demo.js
Modified	samples/stock/demo/orderbook-chart/demo.js
Added	samples/stock/demo/orderbook-chart/demo.js

[pawelfus]

Nice!

In addition to inline comments:

• tooltip should be updated, to reflect what is being displayed, not random X-values and negative numbers
  
• fix the black line in the middle of the chart
• did you consider using category Axis instead of translated dataLabels?

[kamil-musialowski]

@pawelfus Thank you!

• tooltip done
• black line looks like a bug, it disappears when we decrease the chart.marginTop (fixed with crisp)
• I did consider using category axis, but felt more comfortable with dataLabels, since both price and Bids/Asks are values strictly correlated with the point, and axes don't matter on this chart that much

[pawelfus]

Neat! Just a few minor things:

• Is it Orderbook or Order Book? I mean the title.
• Exporting throws errors
• Missing a11y chart's descriptions (in chart config and in demo itselft)
• Let's improve the labels further:

• enable the thousands-separator
• if the righthand labels have decimal point, then the left hand should too. I would say XXX XXX.00 and XXX XXX.50 should be fine. Perhaps one decimal place could be better, hard to say

[kamil-musialowski]

In terms of "Orderbook" vs "Order book", I've seen both but the second one seems to be more popular and correct, therefore I went with this one. Thanks :)

[DJTechnoo]

Nice orderbook demo and great work! 👍
Just a visual thing: When the asks exceed 1 million increases order of magnitude, the data label jumps and creates visual noise. On the bids side it looks fine and steady.
Other than that, see small comment.

To discuss: Does it update overwhelmingly fast, or is it intended to work this way?

[pawellysy]

Looks great! Good job 🎉

[kamil-musialowski]

@DJTechnoo Yes, it should update that fast :D
For the dataLabels, yeah that's the case of the right alignment, and I'm not sure if that can be changed to be honest, but doesn't look bad imo - what do you think @pawelfus ?

[pawelfus]

I saw that too. It's probably a matter of preference, but I would also rather see it without jumping. However, I'm not sure how to fix it. Maybe @TorsteinHonsi has an idea

[pawelfus]

Nitpicking 🧵

[TorsteinHonsi]

Thanks, it looks great! Some change requests inline.

[TorsteinHonsi]

I saw that too. It's probably a matter of preference, but I would also rather see it without jumping. However, I'm not sure how to fix it. Maybe TorsteinHonsi has an idea

@pawelfus Not a good one. Clearly it comes from re-aligning to the right. But instead of keeping the right edge fixed, it animates the x. If someone wants to dig int, feel free. The issue may lie either in the data labels logic, or deeper in the SVGLabel/SVGElement itself.

[pawelfus]

Good point. It seems to be exactly as you said: aligning items animates x. Simple demo: https://jsfiddle.net/BlackLabel/tbcroaeg/

We could disable animations for dataLabels, but accidentally we missed that (we can disable the Chart's animation, but that's not the point of this demo). Setting label.placed = false works around the issue: https://jsfiddle.net/BlackLabel/tbcroaeg/1/

However, I don't think it's a good & clean workaround to include in the official demo. Until we support nice labels/numbers animations should we just set a hard max for the value? To be 999 999? And hard min (100 000)?

[kamil-musialowski]

Merge #20993 first, before merging the demo.

[TorsteinHonsi]

@pawelfus @kamil-musialowski I merged in the fix for the jumping right label, but it turns out it is still jumping. Any ideas? It probably still has to do with the alignment, but the animateX flag returns true for these cases.

[kamil-musialowski]

I had a look into that, but I couldn't come up with anything reasonable 🤔

[pawelfus]

@pawelfus @kamil-musialowski I merged in the fix for the jumping right label, but it turns out it is still jumping. Any ideas? It probably still has to do with the alignment, but the animateX flag returns true for these cases.

Found the reason: https://jsfiddle.net/BlackLabel/zf1q7cdb/2/

In short, we have two align() calls for each label in the demo. The first one aligns according to x+y options. The second one comes from justifyDataLabel() - where we align to the plotting area. That means x option always changes, so we can observe the animation.

[TorsteinHonsi]

Another problem is that the right-justified data label actually has align: 'left', so it effectively ducks our animation fix. Trying out some alternative approach here: #21093

[TorsteinHonsi]

@pawelfus I give up :( Any more ideas? Or should we modify the data so we don't get this situation?

[TorsteinHonsi]

Wait a minute, just want to try one more thing, to implement alignTo for other series than pie. In addition to fixing this issue, the config will also look much cleaner, and avoid the hacky nature of setting x to -999 or 999.

Draft PR: #21117
Stock orderbook with the new option applied: https://jsfiddle.net/highcharts/zaou38t4/

[kamil-musialowski]

@TorsteinHonsi Thank you, the alignTo looks perfect. I will merge the master and add the property to the demo, once #21117 is merged.

[TorsteinHonsi]

@kamil-musialowski The alignTo option is now in the master.

[kamil-musialowski]

Perfect, thank you!