PolymerElements/paper-ripple

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 and width2 to use TargetRadius instead, if this behavior is desired.
  • The variable be removed as it is not needed.