-
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(chips): Extend ripple to fill the chip when animating width #2423
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2423 +/- ##
==========================================
- Coverage 98.82% 98.67% -0.15%
==========================================
Files 102 102
Lines 4079 4090 +11
Branches 511 512 +1
==========================================
+ Hits 4031 4036 +5
- Misses 48 54 +6
Continue to review full report at Codecov.
|
For context, this is an alternative to #2406 which seeks to resolve the issue entirely within mdc-chips rather than needing to add an optional parameter to MDC Ripple. While this avoids affecting Ripple APIs and is less code overall than #2406, it requires adding more logic to a component to effectively patch another component's adapter, and if any other component were to need special treatment, they'd have to repeat this logic. I'm potentially still leaning towards #2406, but I'll see what others think. |
Discussed offline with @kfranqueiro. There are now 6 components (including this one) that override ripple adapter methods, so it may be worth parameterizing the ripple's constructor, not just the In the meantime this PR is a fix adhering to the pattern in the other components. |
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.
Changes LGTM. My one reservation is I'm a bit concerned about the effect on our test coverage if we can't easily test this.
Fixes #2334.