leeoniya / uPlot

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

Add feature for drawing grids over series #391

Closed zefirka closed 3 years ago

zefirka commented 3 years ago

Resolves #386

leeoniya commented 3 years ago

thanks!

i was thinking about this.

really this is not just about the grid, but axes + grid, since a single function draws both. i chose to have a hook called drawAxes since it seemed semantically more important (and axes encapsulate grid draw & opts), so probably want to stick with uniform nomenclature.

grid.over gives the impression that it can be controlled per axis, but it really can't, so duplicating this setting is a bit odd.

i don't see much value in doing the complex deferred thing. we can just swap the call order inside of commit. my general feeling is to have some kind of opts.drawOrder: ["axes","series"], which would control this, and leave the possibility to add more stuff in the future like annotations, etc. i'm not too thrilled about mapping strings in that array to the draw functions, but doing it as an object doesnt communicate ordering as well opts.drawOrder: {grid: 0, axes: 1} and would still require internal prep to convert them to an array of functions that can be looped over.

throughts?

leeoniya commented 3 years ago

let's morph this PR to what's described above.

https://github.com/leeoniya/uPlot/blob/005b3e0903444521f592c14cf33ab922d6e364ca/dist/uPlot.esm.js#L2633-L2639

this is probably all that's needed:

// somewhere in the uPlot closure
const drawOrder = opts.drawOrder || ["axes", "series"];

// inside _commit
drawOrder.forEach(key => {
  key == "axes" && drawAxesGrid(); 
  key == "series" && dataLen > 0 && drawSeries();
});

please leave the build files out of this PR for easier review. i'll run a build once this is merged.

zefirka commented 3 years ago

Hi @leeoniya, yea I was thinking about single option to specify drawing order for axes and series but did not risk to include new hight-level namespace for Options.

Generally, I see option named axesOverSeries: boolean more self-explanatory because of order of drawing is something incapsulated in library and can't describe what will change in result if I will reorder array. What do you think about it?

If you think that it should be drawOrder: ['axes' | 'series'] then I'll do it in this PR, no problems.

please leave the build files out of this PR for easier review. i'll run a build once this is merged.

Sure!

leeoniya commented 3 years ago

Generally, I see option named axesOverSeries: boolean more self-explanatory because of order of drawing is something incapsulated in library and can't describe what will change in result if I will reorder array. What do you think about it?

i think it's too specific. what if we add annotations to uPlot in the future, then we'll need annotationsOverSeriesButUnderGrid: boolean :rofl: ? drawOrder is both shorter and more generic, so we can expand it easily without any ambiguity or piling on more options that also control draw order.

zefirka commented 3 years ago

Fair enough 👍 . I'll change it to drawOrder

leeoniya commented 3 years ago

thanks!

leeoniya commented 3 years ago

decided to go with DrawOrderKey. also adjusted some styling in the demo to make the grid more visible.

builds are pushed.

o-3000 commented 1 year ago

Hello - good work on this feature. Does this also help with changing the order the opts.plugins are executed?

For example in a heatmap which doesn't use the current examples that use series derived data as input to uPlot, but rather uses direct canvas Path method of drawing the heatmap rectangles inside a plugin in this example, can we force the plugins to utilize this drawOrder concept?

leeoniya commented 1 year ago

can we force the plugins to utilize this drawOrder concept?

plugins are not really a concrete thing, they're just a thin pattern over setting up hooks and customizing or tweaking options during init. so what you're asking for is to customize the hook firing order?

please open a new issue with specific data, code and a description or example image of what you're looking to accomplish.

o-3000 commented 1 year ago

hi @leeoniya I see your point and will do submit separate ticket. For now, I found a work around but if my workaround presents limitations I'll definitely open new ticket.

I did a workaround with HTML Canvas property as follows:

// ctx.globalCompositeOperation = 'destination-over'; //like z-index : -1 - uncomment this code if you want to draw path behind other current drawn paths. I assume as we add more elements this may become problematic as we won't have a true z-index ordering method.

leeoniya commented 1 year ago

also, if you are just trying to have a legend order that is different than the series drawing order, i would instead recommend applying display: flex and order css properties to the legend DOM to reorder the labels, but leave the series in natural draw order.

hard to make recommentations without a concrete end goal.

o-3000 commented 1 year ago

Sounds good @leeoniya will open new ticket if have specific use case question or issue report.