leeoniya / uPlot

📈 A small, fast chart for time series, lines, areas, ohlc & bars
MIT License
8.51k stars 371 forks source link

Added candlestick plugin example #116

Closed ldstein closed 4 years ago

ldstein commented 4 years ago

Here is a simple candlestick chart plugin demo as discussed in #98.

What it is doing:

If there is a better way to not draw the series lines while retaining the legend, please let me know.

image

image

leeoniya commented 4 years ago

wow, this looks great! i really like how simple/straightforward all these plugins are turning out, too.

the canvas hack is not great. it wastes a ton of cycles doing work that's useless in the end.

If there is a better way to not draw the series lines while retaining the legend, please let me know.

we should def add an opt for this; series.draw, perhaps?

leeoniya commented 4 years ago

i'd prefer the general aesthetic to be closer to below, (where the body and shadow have a single-thickness [relatively thin] outline):

image

image

leeoniya commented 4 years ago

i've added series.draw in eeeea07534f01d0282cdd3f5d968457a1eb9ab9d, so all the imageData hacks can be dropped. 🎉

also...

ldstein commented 4 years ago

Nice. series.draw does the trick.

PR Updated:

image

leeoniya commented 4 years ago

again, wow. but, that's quite the line-count increase! some thoughts:

tooltip:

it frequently obscures the actual trend data, doing more harm than good. this is the kind of complexity you run into when trying to arrange labels near the datapoints, and is the reason that the tooltip demos in this repo generally follow the cursor along y - allowing you to move it out of the way e.g. upwards.

i'm not a huge fan of transition being applied to transform - it makes the tooltip appear to lag. if you still prefer to keep it, 0.1s seems more palatable.

to me, the tooltip & legend are redundant. since in this case, per-series toggling is not a concern, we can instead simply detach the legend, move it into .wrap, then add position: absolute, top: 0, left: 0 (or better yet, add a class that does this as part of a bigger restyle), remove .inline from it (to allow it to stack), now the legend can be used as the tooltip itself, with value updating already handled! i would still do this as its own plugin if possible, "legendAsTooltipPlugin" or something. hopefully this route kills two birds - reducing line count/complexity, and redundant info.

// Ensure the tooltip fits within the right boundary

can we piggyback off of placement.js instead of adding more code noise to DIY this? for reference, see how https://leeoniya.github.io/uPlot/demos/cursor-tooltip.html does it.

cursor:

if we have the tooltip follow the cursor along y (and snap along x to match idx), the crosshair just adds visual noise, especially when it overlaps the tooltip. maybe just kill it with:

cursor: {
  points: false,
  x: false,
  y: false,
},

general:

i think initial styles can be assigned with less code noise as below, though i'd avoid using this for perf-sensitive props that update on mousemove, like style.transform.

Object.assign(ttEl.style, {
  position: "absolute",
  ...
})

overall, fantastic work on this!

ldstein commented 4 years ago

Cool, thanks for the feedback. PR Updated.

Highlight Plugin and Candlestick plugin

Tooltip Plugin:

Some observations:

setSize hooks fire before init hooks. Not 100% sure if this is intentional. I personally found it unexpected. I kind of assumed init hooks would always execute first.

Should a plugin be able to manipulate opts or configuration prior to initialization? In candleStickPlugin's init hook, I tried disabling the cursor crosshairs by setting:

u.cursor.x.show = false
u.cursor.y.show = false;

However, this won't work because the init plugin hook is triggered after uPlot has already processed the user-defined options (and in this case, added the unwanted crosshairs divs to the DOM).

Maybe the init hook should fire after uPlot has merged user-defined and default options? This will allow plugins a chance to add any additional relevant configuration.

image

leeoniya commented 4 years ago

thanks, looking good. last few comments:

setSize hooks fire before init hooks.

looks like i forgot to skip firing it during init (via ready &&). now fixed in 5a3fc19ed7b9e96583e539306c443572e841f4ad.

https://github.com/leeoniya/uPlot/blob/7f620f40da360e81f314910362e3db401bf8d2d5/src/Line.js#L337-L339

I kind of assumed init hooks would always execute first...Maybe the init hook should fire after uPlot has merged user-defined and default options? This will allow plugins a chance to add any additional relevant configuration.

like all other hooks, init fires after the initialization is complete (options are processed and defaulted, scales are initialized (but not ranged yet), dom is created, event handlers are attached). the issue is that initialization itself requires calling many of the same hook-firing functions as would execute during post-init normal operation, but if i let them fire as part of init, then you cannot rely on e.g. the dom being fully formed. it's a bit of chicken/egg problem, so i intentionally skip firing them until the same expectations can be met on every hook invocation.

Should a plugin be able to manipulate opts or configuration prior to initialization?

i've opened #120 for this. until that's figured out, let's set the cursor opts on the chart directly like you do now.

cursor: {
  x:false,
  y:false
},

you're missing points: false here. even though you did not assign a color to them via series.stroke, without points: false they're still added to the dom and get their styles updated on every setCursor call.

function destroy(u) {

i don't think we need the destroy hook if all it does is related to dom cleaning within the uplot container, since u.destroy() will remove the whole element, the browser should take care of cleaning event listeners. the destroy hook is good for cleaning up anything global/external (e.g. event listeners added to the document level, etc.).

const { left, top } = u.cursor;
const bound = u.ctx.canvas;
const bbox = bound.getBoundingClientRect();
const anchor = { left: left + bbox.left, top: top + bbox.top };

.getBoundingClientRect() is an expensive op. can we cache the bounds and avoid calling this on every setCursor? you can see that the original tooltip demo only queries it on init: https://github.com/leeoniya/uPlot/blob/master/demos/cursor-tooltip.html#L50-L52

u.ctx.canvas.parentElement

this appears often and is effectively static. can we alias it to let plotEl = u.ctx.canvas.parentElement to reduce the wordiness?

onMouseDown
onMouseUp

i'm not sure it's critical to show/hide the highlight during zooming (i don't pause hoverpoint interactions during zooming). i think removing this will simplify the code a good amount. my goal with the demos is to provide minimal/clear/core starting points to build upon.

i'm good to merge once the above is addressed, (and please squash into single commit).

ldstein commented 4 years ago

Note triggering getBoundClientRect during setCursor was intentional. Otherwise, would need to calculate an additional offset whenever chart is scrolled partially offscreen. Not so apparent in cursor-placement.html because it is a narrower chart. More apparent in the wider charts. I'm sure there are other ways to solve this, but unfortunately a little time limited right now.

Below is an example of placement issue when page is horizontally scrolled: image

leeoniya commented 4 years ago

great, thanks!

i'll merge this tomorrow with the plugin changes from https://github.com/leeoniya/uPlot/issues/120#issuecomment-583342183.

leeoniya commented 4 years ago

i've simplified it quite a bit i think:

https://leeoniya.github.io/uPlot/demos/candlestick-ohlc.html

it removes a few things you had in there like setSize handling, but i think it's an easier read for a demo. i've also switched to a simple tooltip impl without external dependencies but also without bounds checks - it works well enough.

now to adjust the plugin api for opts adjustment. this will allow these plugins here to work more independently.

leeoniya commented 4 years ago

plugin api changes are done.

plugins now return {opts, hooks}. each hook can be an array or function, though i don't see much need for the former.