Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

this.maxRadius variable is misleading. #17

Open
Quenty opened this issue May 19, 2015 · 0 comments
Open

this.maxRadius variable is misleading. #17

Quenty opened this issue May 19, 2015 · 0 comments

Comments

@Quenty
Copy link

Quenty commented May 19, 2015

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.
notwaldorf added a commit that referenced this issue May 22, 2015
@notwaldorf notwaldorf self-assigned this Jun 18, 2015
@notwaldorf notwaldorf added this to the 2.0 milestone Jul 12, 2016
@notwaldorf notwaldorf removed their assignment Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants