-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(chips): Don't trigger ripple from trailing icon #2286
Changes from 5 commits
1d7ee3c
85c9ee6
0792944
7acd812
ceb19dd
99a82e9
a35f074
865134a
97b801b
0441134
dd0b8a2
65fa1a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,10 @@ class MDCChipFoundation extends MDCFoundation { | |
hasClass: () => {}, | ||
registerInteractionHandler: () => {}, | ||
deregisterInteractionHandler: () => {}, | ||
registerTrailingIconInteractionHandler: () => {}, | ||
deregisterTrailingIconInteractionHandler: () => {}, | ||
notifyInteraction: () => {}, | ||
notifyTrailingIconInteraction: () => {}, | ||
}); | ||
} | ||
|
||
|
@@ -59,17 +62,27 @@ class MDCChipFoundation extends MDCFoundation { | |
|
||
/** @private {function(!Event): undefined} */ | ||
this.interactionHandler_ = (evt) => this.handleInteraction_(evt); | ||
/** @private {function(!Event): undefined} */ | ||
this.trailingIconInteractionHandler_ = (evt) => this.handleTrailingIconInteraction_(evt); | ||
} | ||
|
||
init() { | ||
['click', 'keydown'].forEach((evtType) => { | ||
this.adapter_.registerInteractionHandler(evtType, this.interactionHandler_); | ||
this.adapter_.registerTrailingIconInteractionHandler(evtType, this.trailingIconInteractionHandler_); | ||
}); | ||
['touchstart', 'pointerdown', 'mousedown'].forEach((evtType) => { | ||
this.adapter_.registerTrailingIconInteractionHandler(evtType, this.trailingIconInteractionHandler_); | ||
}); | ||
} | ||
|
||
destroy() { | ||
['click', 'keydown'].forEach((evtType) => { | ||
this.adapter_.deregisterInteractionHandler(evtType, this.interactionHandler_); | ||
this.adapter_.deregisterTrailingIconInteractionHandler(evtType, this.trailingIconInteractionHandler_); | ||
}); | ||
['touchstart', 'pointerdown', 'mousedown'].forEach((evtType) => { | ||
this.adapter_.deregisterTrailingIconInteractionHandler(evtType, this.trailingIconInteractionHandler_); | ||
}); | ||
} | ||
|
||
|
@@ -93,6 +106,18 @@ class MDCChipFoundation extends MDCFoundation { | |
this.adapter_.notifyInteraction(); | ||
} | ||
} | ||
|
||
/** | ||
* Handles an interaction event on the trailing icon element. This is used to | ||
* prevent the ripple from activating on interaction with the trailing icon. | ||
* @param {!Event} evt | ||
*/ | ||
handleTrailingIconInteraction_(evt) { | ||
evt.stopPropagation(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we eventually going to emit our own event on trailing icon click? I'm very wary of anything that adds There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if (evt.type === 'click' || evt.key === 'Enter' || evt.keyCode === 13) { | ||
this.adapter_.notifyTrailingIconInteraction(); | ||
} | ||
} | ||
} | ||
|
||
export default MDCChipFoundation; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,9 @@ test('exports cssClasses', () => { | |
test('defaultAdapter returns a complete adapter implementation', () => { | ||
verifyDefaultAdapter(MDCChipFoundation, [ | ||
'addClass', 'removeClass', 'hasClass', | ||
'registerTrailingIconInteractionHandler', 'deregisterTrailingIconInteractionHandler', | ||
'registerInteractionHandler', 'deregisterInteractionHandler', 'notifyInteraction', | ||
'notifyTrailingIconInteraction', | ||
]); | ||
}); | ||
|
||
|
@@ -48,6 +50,11 @@ test('#init adds event listeners', () => { | |
|
||
td.verify(mockAdapter.registerInteractionHandler('click', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.registerInteractionHandler('keydown', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.registerTrailingIconInteractionHandler('click', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.registerTrailingIconInteractionHandler('keydown', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.registerTrailingIconInteractionHandler('touchstart', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.registerTrailingIconInteractionHandler('pointerdown', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.registerTrailingIconInteractionHandler('mousedown', td.matchers.isA(Function))); | ||
}); | ||
|
||
test('#destroy removes event listeners', () => { | ||
|
@@ -56,6 +63,11 @@ test('#destroy removes event listeners', () => { | |
|
||
td.verify(mockAdapter.deregisterInteractionHandler('click', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.deregisterInteractionHandler('keydown', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.deregisterTrailingIconInteractionHandler('click', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.deregisterTrailingIconInteractionHandler('keydown', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.deregisterTrailingIconInteractionHandler('touchstart', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.deregisterTrailingIconInteractionHandler('pointerdown', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.deregisterTrailingIconInteractionHandler('mousedown', td.matchers.isA(Function))); | ||
}); | ||
|
||
test('#toggleActive adds mdc-chip--activated class if the class does not exist', () => { | ||
|
@@ -90,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might be able to save some code here by using (You can pass additional properties to the functions it returns, to be assigned to the event object.) Also, should we actually stub |
||
const {foundation, mockAdapter} = setupTest(); | ||
const mockEvt = { | ||
type: 'click', | ||
stopPropagation: () => {}, | ||
}; | ||
let click; | ||
|
||
td.when(mockAdapter.registerTrailingIconInteractionHandler('click', td.matchers.isA(Function))) | ||
.thenDo((evtType, handler) => { | ||
click = handler; | ||
}); | ||
|
||
foundation.init(); | ||
click(mockEvt); | ||
|
||
td.verify(mockAdapter.notifyTrailingIconInteraction()); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,6 +78,36 @@ test('#adapter.deregisterInteractionHandler removes event listener for a given e | |
td.verify(handler(td.matchers.anything()), {times: 0}); | ||
}); | ||
|
||
test('#adapter.registerTrailingIconInteractionHandler adds event listener for a given event to the trailing' + | ||
'icon element', () => { | ||
const {root, component} = setupTest(); | ||
const icon = bel` | ||
<i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0" role="button">cancel</i> | ||
`; | ||
root.appendChild(icon); | ||
const handler = td.func('click handler'); | ||
component.getDefaultFoundation().adapter_.registerTrailingIconInteractionHandler('click', handler); | ||
domEvents.emit(icon, 'click'); | ||
|
||
td.verify(handler(td.matchers.anything())); | ||
}); | ||
|
||
test('#adapter.deregisterTrailingIconInteractionHandler removes event listener for a given event from the trailing ' + | ||
'icon element', () => { | ||
const {root, component} = setupTest(); | ||
const icon = bel` | ||
<i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0" role="button">cancel</i> | ||
`; | ||
root.appendChild(icon); | ||
const handler = td.func('click handler'); | ||
|
||
icon.addEventListener('click', handler); | ||
component.getDefaultFoundation().adapter_.deregisterTrailingIconInteractionHandler('click', handler); | ||
domEvents.emit(icon, 'click'); | ||
|
||
td.verify(handler(td.matchers.anything()), {times: 0}); | ||
}); | ||
|
||
test('#adapter.notifyInteraction emits ' + | ||
`${MDCChipFoundation.strings.INTERACTION_EVENT}`, () => { | ||
const {component} = setupTest(); | ||
|
@@ -90,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: You could omit the template string syntax here since it's already concatenated anyway |
||
const {component} = setupTest(); | ||
const handler = td.func('interaction handler'); | ||
|
||
component.listen( | ||
MDCChipFoundation.strings.TRAILING_ICON_INTERACTION_EVENT, handler); | ||
component.getDefaultFoundation().adapter_.notifyTrailingIconInteraction(); | ||
|
||
td.verify(handler(td.matchers.anything())); | ||
}); | ||
|
||
function setupMockFoundationTest(root = getFixture()) { | ||
const MockFoundationConstructor = td.constructor(MDCChipFoundation); | ||
const mockFoundation = new MockFoundationConstructor(); | ||
|
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.
Double quotes -> backticks