Added events to smoothie charts #116
Conversation
drewnoakes
left a comment
drewnoakes
left a comment
There was a problem hiding this comment.
This is a nice feature, but as it stands this PR cannot be merged. It removes a bunch of functionality others depend upon. I also think the implementation could be improved to have better performance, as discussed in comments below.
| if (typeof dataSet[i][1] === 'string' ) { | ||
| context.moveTo(x, 0); | ||
| context.lineTo(x, dimensions.height); | ||
| context.fillStyle = seriesOptions.strokeStyle; | ||
| context.fillText(dataSet[i][1], x + 5, 10); | ||
| } else { | ||
| context.moveTo(x, y); | ||
| } |
There was a problem hiding this comment.
Rather than do this type check on every point on every series, it'd be clearer and faster to create a new series type for these events.
There was a problem hiding this comment.
Wouldn't the function be exactly the same ? I just think it doubles the code unnecessarily
There was a problem hiding this comment.
The final outcome would be the same, but it's a lot more work to test the type on every single point on all series. Smoothie is light and fast. Currently this cost would be paid by everyone, regardless of whether they use these events. Using a new SmoothieEventSeries type avoids that cost.
francobottero
force-pushed
the
master
branch
2 times, most recently
from
c1820e6 to
b542507
Compare
francobottero
force-pushed
the
master
branch
from
b542507 to
bc47a4b
Compare
|
@francobottero Have you considered updating this to use a separate TimeSeries type? I for one would like to see this feature added. |
|
@cinderblock feel free to create a new PR if you have the bandwidth. |
3 participants
It's important to say that events have to go on a separate
TimeSeriesthan the rest of the data or else, they will interfere with the chart.Events will look something like this :