Closed RichyHBM closed 6 years ago
I think the main problem with merging them is that then the spark line would draw an outline to the fill, if you can think of a way to do it do let me know though and I'll change it!
I'm not entirely sure what you mean. Are you saying for folks who want to have a fill color without a stroke color? I think the path should be the same regardless of how we choose to draw it.
Yes, so if its the same path when we draw the line it will always have an outline when also drawing with a fill, with multiple paths you can have the line drawing one color and the fill another without having an outline around all the fill
So the stroke vs fill is a property of the paint, not the path.
So I'm thinking in all cases we should have a single path representing the sparkline. Then we can do something like:
canvas.drawPath(path, strokePaint)
if (shouldFill) {
canvas.drawPath(path, fillPaint)
}
Yes, but to draw the fill, the path needs to be closed which would mean the stroke paint would always draw an outline (drawing the spark line, and then a line down to 0, and back to the start of the spark)
Yeah, I think that's what we'd want in this case. I think it would look really odd to just have the stroke on the top of the graph instead of all around it. So for fill cases, the sparkpath will be closed, and both filled and stroked.
That would mean the animators would be completely broken with fills, just to confirm, you are OK with this? In fact if we dont stop the fill from drawing when animating, I can't think of any situation in which an animation would work with fill
They're completely broken currently as well, right? Given that it's not a regression, I'm pretty comfortable with it. I'm open to a separate diff that fixes them in some way, but I'm thinking that's out of scope for this PR.
OK, will revert my changes today!
Fixes #37 Adds a new attribute allowing you to set a different color for the fill, it also fixes the animators to work with fill