feat(chips): Allow close icon and exit animation #2571

bonniezhou · 2018-04-16T17:37:41Z

Add remove functionality + animation to chips and hook it up to mdc-chip__icon--remove.
Fixes #2010.

codecov-io · 2018-04-16T18:11:53Z

Codecov Report

Merging #2571 into master will decrease coverage by 0.22%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #2571      +/-   ##
==========================================
- Coverage   98.68%   98.46%   -0.23%     
==========================================
  Files          98       98              
  Lines        4198     4233      +35     
  Branches      533      537       +4     
==========================================
+ Hits         4143     4168      +25     
- Misses         55       65      +10

Impacted Files	Coverage Δ
packages/mdc-chips/chip/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-chips/chip-set/foundation.js	`100% <100%> (ø)`	⬆️
packages/mdc-chips/chip-set/index.js	`95.45% <100%> (+0.45%)`	⬆️
packages/mdc-chips/chip/index.js	`82.14% <66.66%> (+4.14%)`	⬆️
packages/mdc-chips/chip/foundation.js	`97.29% <94.11%> (-0.95%)`	⬇️
packages/mdc-grid-list/index.js	`46.15% <0%> (-53.85%)`	⬇️
packages/mdc-grid-list/foundation.js	`93.75% <0%> (-6.25%)`	⬇️
packages/mdc-menu/foundation.js	`99.02% <0%> (-0.33%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 698e0d4...b269621. Read the comment docs.

kfranqueiro · 2018-04-16T20:43:36Z

packages/mdc-chips/chip/index.js

@@ -125,6 +125,9 @@ class MDCChip extends MDCComponent {
      notifyInteraction: () => this.emit(strings.INTERACTION_EVENT, {chip: this}, true /* shouldBubble */),
      notifyTrailingIconInteraction: () => this.emit(
        strings.TRAILING_ICON_INTERACTION_EVENT, {chip: this}, true /* shouldBubble */),
+      getComputedStyleValue: (propertyName) => window.getComputedStyle(this.root_).getPropertyValue(propertyName),
+      setStyleProperty: (propertyName, value) => this.root_.style.setProperty(propertyName, value),
+      removeFromDOM: () => this.root_.remove(),


This won't work in IE. You're going to have to do this.root_.parentNode.removeChild(this.root_)

kfranqueiro · 2018-04-16T20:47:22Z

packages/mdc-chips/chip/mdc-chip.scss

+.mdc-chip--exit {
+  transition:
+    opacity 75ms $mdc-animation-standard-curve-timing-function,
+    width 150ms $mdc-animation-deceleration-curve-timing-function,


Does this need to be width/padding/margin or can we do a scale transform instead, to take advantage of GPU compositing and eliminate potential for text truncation/wrapping issues?

I'm also a tad confused why we have the non-opacity transitions taking longer than the opacity (i.e. the element will already be invisible at that point).

Discussed offline - transitioning the transform would be extremely complicated due to chips being wrapped to the next line.

kfranqueiro · 2018-04-19T14:52:11Z

packages/mdc-chips/chip/foundation.js

   * @param {!Event} evt
   */
  handleTransitionEnd_(evt) {
+    // Handle transition end event on the chip when the it is about to be removed.


"when the it" -> "when it"

kfranqueiro · 2018-04-19T14:56:54Z

packages/mdc-chips/chip/foundation.js

+    // Handle transition end event on the chip when the it is about to be removed.
+    if (this.adapter_.eventTargetHasClass(/** @type {!EventTarget} */ (evt.target), cssClasses.CHIP_EXIT)) {
+      if (evt.propertyName === 'width') {
+        this.adapter_.removeFromDOM();


Should we also be calling this.destroy() here? AFAIK we're never destroying these chips even though they're effectively removed.

Also, just checking, chip set isn't holding any references to entry chips, is it? (I think it only holds references to selected chips for filter/choice.)

Oh that's a really good point, the chip set does hold a reference to all its chips, regardless of the type of chip set.

I refactored it so that the chip set is in charge of removing the chip object from its reference, and also calls remove on the chip, which will destroy and remove itself from the DOM.

Note that this means the chip set foundation has to pass a chip object to its adapter, and the chip object needs to have an explicit remove method.

kfranqueiro · 2018-04-19T21:09:47Z

packages/mdc-chips/README.md

@@ -217,6 +219,9 @@ Method Signature | Description
 `deregisterTrailingIconInteractionHandler(evtType: string, handler: EventListener) => void` | Deregisters an event listener on the trailing icon element
 `notifyInteraction() => void` | Emits a custom event `MDCChip:interaction` denoting the chip has been interacted with
 `notifyTrailingIconInteraction() => void` | Emits a custom event `MDCChip:trailingIconInteraction` denoting the chip's trailing icon has been interacted with
+`notifyRemoval() => void` | Emits a custom event `MDCChip:removal` denoting the chip will be removed


Is there a reason you preferred removal over remove (or removed) here?

Just using a noun to mirror "interaction". Is "remove" more understandable?

Oh, I guess that's true. I was thinking of e.g. "selected" on some other components. I guess removal works here.

kfranqueiro · 2018-04-19T21:29:55Z

packages/mdc-chips/README.md

@@ -229,6 +234,7 @@ Method Signature | Description
 `deregisterInteractionHandler(evtType: string, handler: EventListener) => void` | Deregisters an event handler on the root element for a given event
 `createChipElement(text: string, leadingIcon: Element, trailingIcon: Element) => Element` | Returns a chip element with the given text, leading icon, and trailing icon
 `appendChild(el: Element) => void` | Appends the given element as a child of the root element
+`removeChip(chip: MDCChip) => void` | Removes the chip object from the chip set


Do you think it would make sense to combine createChipElement and appendChild into a single appendChip API, to mirror removeChip? (I think those other two are only ever used together back to back anyway?)

Yeah that's a really good idea

kfranqueiro · 2018-04-19T21:33:18Z

test/unit/mdc-chips/mdc-chip.test.js

@@ -49,6 +49,16 @@ test('get ripple returns MDCRipple instance', () => {
  assert.isTrue(component.ripple instanceof MDCRipple);
 });

+test('#remove removes the root element from the DOM', () => {


Is there a way for us to also test that destroy is run?

kfranqueiro · 2018-04-19T21:34:05Z

test/unit/mdc-chips/mdc-chip-set.test.js

+  const component = new MDCChipSet(root, undefined, (el) => new FakeChip(el));
+  const chip = component.chips[0];
+  component.getDefaultFoundation().adapter_.removeChip(chip);
+  td.verify(chip.remove());


Also verify that component.chips' length is 0?

Done. (It should have length 2 since we're only removing 1 chip)

kfranqueiro · 2018-04-19T23:33:46Z

packages/mdc-chips/chip-set/foundation.js

@@ -83,8 +91,7 @@ class MDCChipSetFoundation extends MDCFoundation {
   * @return {!Element}
   */
  addChip(text, leadingIcon, trailingIcon) {
-    const chipEl = this.adapter_.createChipElement(text, leadingIcon, trailingIcon);
-    this.adapter_.appendChild(chipEl);
+    const chipEl = this.adapter_.appendChip(text, leadingIcon, trailingIcon);


Do these still need to return the chip element? Would it make more sense to return the chip itself, or nothing at all?

Oops, nevermind, forgot the component needs it.

The foundation needs to return the chip element so the component can push it to its array of chip references.

kfranqueiro · 2018-04-19T23:40:49Z

test/unit/mdc-chips/mdc-chip-set.foundation.test.js

@@ -184,3 +185,20 @@ test('in filter chips, on custom MDCChip:interaction event deselects selected ch
  td.verify(chipA.foundation.setSelected(false));
  assert.equal(foundation.selectedChips_.length, 0);
 });
+
+test('on custom MDCChip:removal event removes chip', () => {


Do we not have a test in this file for addChip? (We're testing it at the component level, but IIUC that's mocking the foundation, so we should be testing it here too.)

kfranqueiro · 2018-04-19T23:41:19Z

test/unit/mdc-chips/mdc-chip-set.test.js

-test('#adapter.createChipElement returns a new chip element', () => {
-  const {component} = setupTest();
-  const chipEl = component.getDefaultFoundation().adapter_.createChipElement('hello world');
+test('#adapter.appendChip add a new chip to the chip set element', () => {


add -> adds

bonniezhou and others added 6 commits April 10, 2018 15:06

WIP exit animation

4394bd4

get exit animation working yay

cc7548a

Make smoother and only on close icon

476eaa7

Merge branch 'master' into feat/chips/exit-animation

85c2067

Clean and lint

7b2176c

Update readme and rename remove icon

987e611

bonniezhou and others added 3 commits April 16, 2018 12:53

Add tests

af7d9fe

Lint

4822b09

Merge branch 'master' into feat/chips/exit-animation

53a631f

bonniezhou requested a review from kfranqueiro April 16, 2018 20:25

kfranqueiro suggested changes Apr 16, 2018

View reviewed changes

Fix for IE

406b6c4

kfranqueiro force-pushed the master branch from 41f496c to 6f97632 Compare April 17, 2018 21:22

Merge branch 'master' into feat/chips/exit-animation

86afc77

kfranqueiro suggested changes Apr 19, 2018

View reviewed changes

bonniezhou and others added 3 commits April 19, 2018 12:28

Typo

ec91229

Make chip set handle removing chip

43abae2

Merge branch 'master' into feat/chips/exit-animation

f2c7b17

kfranqueiro suggested changes Apr 19, 2018

View reviewed changes

bonniezhou and others added 3 commits April 19, 2018 15:27

Collapse createChipElement and appendChild into appendChip

abaaee9

Merge branch 'master' into feat/chips/exit-animation

5157bb9

Test that destroy was called

e2b7530

kfranqueiro reviewed Apr 19, 2018

View reviewed changes

kfranqueiro suggested changes Apr 19, 2018

View reviewed changes

bonniezhou and others added 2 commits April 19, 2018 17:02

Tests

4f403de

Merge branch 'master' into feat/chips/exit-animation

4948f5c

kfranqueiro approved these changes Apr 20, 2018

View reviewed changes

bonniezhou added 2 commits April 20, 2018 11:14

Merge branch 'master' into feat/chips/exit-animation

87a3347

Merge branch 'master' into feat/chips/exit-animation

b269621

bonniezhou merged commit 3d8a27b into master Apr 20, 2018

bonniezhou deleted the feat/chips/exit-animation branch April 20, 2018 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(chips): Allow close icon and exit animation #2571

feat(chips): Allow close icon and exit animation #2571

bonniezhou commented Apr 16, 2018 •

edited

Loading

codecov-io commented Apr 16, 2018 •

edited

Loading

kfranqueiro Apr 16, 2018

kfranqueiro Apr 16, 2018

bonniezhou Apr 18, 2018

kfranqueiro Apr 19, 2018

kfranqueiro Apr 19, 2018

bonniezhou Apr 19, 2018

kfranqueiro Apr 19, 2018

bonniezhou Apr 19, 2018

kfranqueiro Apr 19, 2018

kfranqueiro Apr 19, 2018

bonniezhou Apr 19, 2018

kfranqueiro Apr 19, 2018

kfranqueiro Apr 19, 2018 •

edited

Loading

bonniezhou Apr 19, 2018

kfranqueiro Apr 19, 2018

kfranqueiro Apr 19, 2018

bonniezhou Apr 19, 2018

kfranqueiro Apr 19, 2018

kfranqueiro Apr 19, 2018

feat(chips): Allow close icon and exit animation #2571

feat(chips): Allow close icon and exit animation #2571

Conversation

bonniezhou commented Apr 16, 2018 • edited Loading

codecov-io commented Apr 16, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonniezhou commented Apr 16, 2018 •

edited

Loading

codecov-io commented Apr 16, 2018 •

edited

Loading

kfranqueiro Apr 19, 2018 •

edited

Loading