Introduce emptyThickness parameter
#167
Conversation
The test "Test circle with value = 0.5 and default fill" was failing on my machine because of color smoothing (maybe antialias?), as the color detected by the test at pixel 1 was #fe0000 instead of #ff0000. However, the visual result was correct. Sampling the second pixel instead fixed the test.
dist/circle-progress.js
Outdated
| @@ -369,21 +378,17 @@ | |||
| drawEmptyArc: function(v) { | |||
| var ctx = this.ctx, | |||
| r = this.radius, | |||
| t = this.getThickness(), | |||
| a = this.startAngle; | |||
| arcR = this.getArcRadius(), | |||
There was a problem hiding this comment.
I don't like that there're this.radius and this.getArcRadious() at the same time.
- Since the radius is not equal to
this.size / 2anymore - I'd get rid ofthis.radius. - Also, I'd rename the method to
this.getRadius(), because it affects both arc & BG circle.
There was a problem hiding this comment.
Yes, I left this.radius just to have easy access to this.size / 2, as I need that in order to calculate the actual radius and the x, y coords for the circle.
I could either calculate the half coords everytime in drawArc and drawEmptyArc, as well as getArcRadius, or rename this.radius to something like this.halfSize, and use that instead of this.radius, just to avoid unnecessary math operations.
What do you think?
There was a problem hiding this comment.
This variable needs to be updated when this.size is changed. I'd rather drop it. There are only 2 places where it'd get replaced with this.size / 2.
Also, I still think it's reasonable to rename your getArcRadius method into getRadius.
I suggest the following naming:
// ...
var c = this.size / 2, // center X/Y coords
r = this.getRadius(),
t = this.getThickness(),
a = this.startAngle;
// ...
ctx.arc(c, c, r, a, a + Math.PI * 2 * v);
dist/circle-progress.js
Outdated
| } else { | ||
| ctx.arc(r, r, r - t / 2, a, a - Math.PI * 2 * v); | ||
| } | ||
| ctx.arc(r, r, arcR, Math.PI * 2, 0); |
There was a problem hiding this comment.
This line, as well as the whole if...else is not needed anymore.
|
@monovertex - do you agree with my recent remarks? |
|
@kottenator Yes I do. I apologize for the delay, it's been a few really busy days at work. I'll fix the required changes as soon as possible. |
|
Great, I'm looking forward for it ;) |
|
@monovertex - ping? ;) |
|
@kottenator, I'm sorry for the really late update. Things got hectic IRL and I haven't had the time or energy to focus on anything else. Let me know if there are any other improvements you'd like me to make to this PR. |
|
@kottenator, ping? |
|
@kottenator, coming back for one more ping, hope you see this some day. Thank you! |
This PR fixes #164.
Adding the
emptyThicknessparameter caused some issues when the empty arc was thicker than the full one (the empty arc had parts rendered outside the canvas), so I had to rewrite the arc rendering logic a bit, in order to use the max thickness between the two arcs.Furthermore, thicker empty arcs also required the full empty circle to be rendered, not only the empty part itself.
I added a new example for the different thickness parameters, but I placed it before the heavily customized circle, even though it comes after it as number 6.
Finally, I added several tests for the new functionality and fixed one that was failing on my laptop.