fix(ripple): Ripple should not activate on disabled label click #532
Conversation
Codecov Report
@@ Coverage Diff @@
## master #532 +/- ##
=========================================
+ Coverage 99.9% 99.9% +<.01%
=========================================
Files 53 53
Lines 2217 2221 +4
Branches 265 266 +1
=========================================
+ Hits 2215 2219 +4
Misses 2 2
Continue to review full report at Codecov.
|
amsheehan
requested review from
yeelan0319
and removed request for
traviskaufman
There was a problem hiding this comment.
Looks like this decreases coverage on a bunch of our files. I think more tests are needed here for the ripple foundation logic as well as making sure in the checkbox/radio components that clicking on a disabled checkbox/radio will not cause the ripple activation classes to be added.
packages/mdc-checkbox/index.js
Outdated
| @@ -64,6 +64,7 @@ export class MDCCheckbox extends MDCComponent { | |||
|
|
|||
| getDefaultFoundation() { | |||
| return new MDCCheckboxFoundation({ | |||
| shouldIgnoreRippleActivation: () => this.disabled, | |||
There was a problem hiding this comment.
Hmm....I'm thinking that isSurfaceDisabled or isSurfaceInteractive might be a better method name here.
With shouldIngoreRippleActivation, that kind of leaves the heuristics of when a ripple should not be triggered up to the implementor. I'm unsure if this is what we should be going for. Instead, we should encode the heuristics of when the ripple should be activated into the foundation itself (e.g. when isSurfaceActive is false and we encounter a keyboard event, we don't enable the ripple).
packages/mdc-radio/README.md
Outdated
| @@ -137,6 +137,8 @@ Since MDC Radio is primarily driven by its native control, the adapter API is ex | |||
| | `addClass(className: string) => void` | Adds a class to the root element. | | |||
| | `removeClass(className: string) => void` | Removes a class from the root element. | | |||
|
|
|||
| > **NOTE**: There are some cases where we do not want ripples to activate. Most of this logic is handled by native control attributes such as `disabled` on input fields. One caveat is that ripples will activate on disabled elements if their corresponding `label` element is clicked. For this, we override the ripple's `shouldIgnoreRippleActivation()` adapter method in `mdc-radio`'s `getDefaultFoundation()`, and check if the control is disabled before activating the ripple. | |||
There was a problem hiding this comment.
What's the reason for including this note here? Seems out of context with the rest of the README. If this is advice for how framework implementors can deal with configuring the ripple properly, we may want to give more background on that and also mention this within the mdc-checkbox README.
There was a problem hiding this comment.
This actually is no longer applicable. The latest code keeps all of this logic in mdc-ripple, whose README documents the behavior.
amsheehan
force-pushed
the
fix/ripple-fires-on-disabled
branch
from
03cb094 to
92677e6
Compare
amsheehan
force-pushed
the
fix/ripple-fires-on-disabled
branch
from
92677e6 to
4d68e60
Compare
resolves #377