fix(chips): Don't trigger ripple from trailing icon #2286
Conversation
| * @param {!Event} evt | ||
| */ | ||
| handleTrailingIconInteraction_(evt) { | ||
| evt.stopPropagation(); |
There was a problem hiding this comment.
Are we eventually going to emit our own event on trailing icon click? I'm very wary of anything that adds stopPropagation with no alternative, since it tends to bite us later.
There was a problem hiding this comment.
Good call! I updated it so the trailing icon emits a custom event.
In a later PR we'll capture this event in the chip and handle it if it's a remove icon.
Codecov Report
@@ Coverage Diff @@
## master #2286 +/- ##
==========================================
+ Coverage 99.04% 99.04% +<.01%
==========================================
Files 99 99
Lines 3976 3999 +23
Branches 510 513 +3
==========================================
+ Hits 3938 3961 +23
Misses 38 38
Continue to review full report at Codecov.
|
|
Eventually having an option in ripple to ignore sub-elements sounds like a good idea. |
test/unit/mdc-chips/mdc-chip.test.js
Outdated
| @@ -120,6 +120,18 @@ test('#adapter.notifyInteraction emits ' + | |||
| td.verify(handler(td.matchers.anything())); | |||
| }); | |||
|
|
|||
| test('#adapter.notifyTrailingIconInteraction emits ' + | |||
| `${MDCChipFoundation.strings.INTERACTION_EVENT}`, () => { | |||
There was a problem hiding this comment.
Nit: You could omit the template string syntax here since it's already concatenated anyway
| @@ -101,3 +102,22 @@ test('on click, emit custom event', () => { | |||
|
|
|||
| td.verify(mockAdapter.notifyInteraction()); | |||
| }); | |||
|
|
|||
| test('on click in trailing icon, emit custom event', () => { | |||
There was a problem hiding this comment.
You might be able to save some code here by using test/unit/helpers.captureHandlers. Example usage within ripple: https://github.com/material-components/material-components-web/blob/master/test/unit/mdc-ripple/foundation-activation.test.js#L27
(You can pass additional properties to the functions it returns, to be assigned to the event object.)
Also, should we actually stub stopPropagation and test that it gets called? (I realize right now it needs to exist simply to not throw an error in the first place, but it could also test unintended behavior changes later.)
packages/mdc-chips/README.md
Outdated
| `notifyInteraction() => void` | Emits a custom event "MDCChip:interaction" denoting the chip has been interacted with, which bubbles to the parent `mdc-chip-set` element | ||
| `notifyTrailingIconInteraction() => void` | Emits a custom event "MDCChip:trailingIconInteraction" denoting the chip's trailing icon has been interacted with, which bubbles to the parent `mdc-chip-set` element |
There was a problem hiding this comment.
Double quotes -> backticks
test/unit/mdc-chips/mdc-chip.test.js
Outdated
| @@ -90,6 +119,17 @@ test('#adapter.notifyInteraction emits ' + | |||
| td.verify(handler(td.matchers.anything())); | |||
| }); | |||
|
|
|||
| test('#adapter.notifyTrailingIconInteraction emits ' + MDCChipFoundation.strings.INTERACTION_EVENT, () => { | |||
There was a problem hiding this comment.
INTERACTION_EVENT -> TRAILING_ICON_INTERACTION_EVENT
|
Thanks, fixed! |
Resolves #2266.
A trailing icon should never trigger a ripple because its behaviour should always be separate from the chip's.