this.maxRadius variable is misleading.
Opened this issue · 0 comments
The this.maxRadius
variable is a misleading variable. For one thing, it is only used in 2 calculations, both of which are state checks:
get isOpacityFullyDecayed() {
return this.opacity < 0.01 &&
this.radius >= Math.min(this.maxRadius, Ripple.MAX_RADIUS);
},
get isRestingAtMaxRadius() {
return this.opacity >= this.initialOpacity &&
this.radius >= Math.min(this.maxRadius, Ripple.MAX_RADIUS);
},
https://github.com/PolymerElements/paper-ripple/blob/master/paper-ripple.html#L303
https://github.com/PolymerElements/paper-ripple/blob/master/paper-ripple.html#L308
It's actual value is based off of the potential radius required for the circle to fill the container
this.maxRadius = this.containerMetrics.furthestCornerDistanceFrom(
this.xStart,
this.yStart
);
https://github.com/PolymerElements/paper-ripple/blob/master/paper-ripple.html#L397
However, it's never used, except in cases where this radius is less than the required amount. In this case, it probably just cuts the animation short, as both isRestingAtMaxRadius
and isOpacityFullyDecayed
are calculations that, quite frankly, used only to determine whether or not to stop animating:
if (ripple.isOpacityFullyDecayed && !ripple.isRestingAtMaxRadius) {
...
https://github.com/PolymerElements/paper-ripple/blob/master/paper-ripple.html#L580
In the actual radius calculation, it is never used, instead, MAX_RADIUS
, a conflicting constant, is used, along with the container metrics. In this case, this variable is not only misleading, but also probably creating behavior that is not wanted (such as having the animation stop before the radius desired is met).
In this case, it's suggested that any of the following be done:
- The variable name be changed to something like
TargetRadius
- The variable actually be used to determine the radius desired, switching the magnitude check that uses
height2
andwidth2
to use TargetRadius instead, if this behavior is desired. - The variable be removed as it is not needed.