-
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(ripple): Clean activation timer and css when interrupted (#2460) #2490
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
Codecov Report
@@ Coverage Diff @@
## master #2490 +/- ##
==========================================
+ Coverage 98.87% 98.87% +<.01%
==========================================
Files 102 102
Lines 4072 4077 +5
Branches 510 511 +1
==========================================
+ Hits 4026 4031 +5
Misses 46 46
Continue to review full report at Codecov.
|
CLA signed |
CLAs look good, thanks! |
@@ -176,6 +176,18 @@ testFoundation('#destroy removes all CSS variables', ({foundation, adapter, mock | |||
}); | |||
}); | |||
|
|||
testFoundation('#destroy clears the activation timer if pending', | |||
({foundation, adapter, mockRaf}) => { |
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 think this indentation and the presence of adapter
when unused is throwing off our linter (check npm run lint
in the repo; npm run fix:js
might help)
@kfranqueiro I have fixed the linting issues as requested, but I amended the original commit instead of making a second one. I hope that’s alright. |
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.
It occurs to me that we should probably also be removing the FG_ACTIVATION class within destroy
now to cover the case where this timeout is canceled (which normally removes the class)... would you be willing to add that and add a test next to the current destroy css class tests?
Also, have you already confirmed for yourself that these changes resolve your issue when using RMWC?
@kfranqueiro I assumed that since the component is destroyed, all relevant classes will be removed with the node. The error was caused by the timeout callback trying to remove the classes from a node that is no longer there. I will look into this and submit a new commit. I have personally confirmed that this PR solves the issue. |
New commit submitted, includes removal of FG_ACTIVATION classes and relevant tests. |
Thanks for working on this. FYI, it looks like you worked directly on your master branch; because we squash and merge when accepting PRs, you'll need to fetch our repo and hard-reset your fork's master branch to match the main repo's history again. Assuming you have the main repo's remote added as
I'd recommend creating new branches for PRs in the future for easier development flow. |
You're welcome. I'm just happy to do my part. |
This commit adds a check in the ripple foundation's destroy function (as suggested by @kfranqueiro) and if the activation timer is active it's cleared. A unit test has also been added.
Fixes #2460.