-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix: Correct unbounded ripple diameter #1098
fix: Correct unbounded ripple diameter #1098
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1098 +/- ##
=========================================
+ Coverage 99.9% 99.9% +<.01%
=========================================
Files 69 69
Lines 3302 3305 +3
Branches 405 405
=========================================
+ Hits 3299 3302 +3
Misses 3 3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@touficbatache Thanks for filing this PR.
I think it overall looks good, could you address the concern in the comment and then I will help merge it.
packages/mdc-ripple/foundation.js
Outdated
@@ -471,7 +471,7 @@ export default class MDCRippleFoundation extends MDCFoundation { | |||
this.frame_ = this.adapter_.computeBoundingRect(); | |||
|
|||
const maxDim = Math.max(this.frame_.height, this.frame_.width); | |||
const surfaceDiameter = Math.sqrt(Math.pow(this.frame_.width, 2) + Math.pow(this.frame_.height, 2)); | |||
const surfaceDiameter = this.frame_.width; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we change this to Math.min(this.frame_.width, this.frame_.height)
so when unbounded ripple is applied to non-square surface, it will always start within the container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sure, done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @touficbatache
@yeelan0319 No problem, happy to be a part of the community 😃 |
This reverts commit 0f1ca35.
This reverts commit 0f1ca35.
Fixes and closes #1067