feat(chips): Replace leading icon with checkmark in selected filter chips #2320

bonniezhou · 2018-02-28T15:40:09Z

Fixes #2009.
When a filter chip is selected, add a checkmark as the leading icon of the filter chip. If a leading icon already exists, replace it with the checkmark.

Known bug: the ripple doesn't extend all the way to the end of the chip when the chip's width is animating. Filed #2334 to be fixed in a later PR.

…hips

codecov-io · 2018-03-02T20:35:19Z

Codecov Report

Merging #2320 into master will increase coverage by 0.02%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##           master    #2320      +/-   ##
==========================================
+ Coverage   98.86%   98.88%   +0.02%     
==========================================
  Files         100      100              
  Lines        4123     4144      +21     
  Branches      528      535       +7     
==========================================
+ Hits         4076     4098      +22     
+ Misses         47       46       -1

Impacted Files	Coverage Δ
packages/mdc-chips/chip/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-chips/chip/index.js	`91.89% <100%> (+9.13%)`	⬆️
packages/mdc-chips/chip/foundation.js	`98.18% <95.23%> (-1.82%)`	⬇️

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 567deec...1a84c1c. Read the comment docs.

williamernest · 2018-03-05T21:06:51Z

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

@@ -39,6 +40,11 @@ function setupTest() {
  return {root, component};
 }

+test('get ripple returns MDCRipple instance', () => {
+  const {component} = setupTest();
+  assert.isOk(component.ripple instanceof MDCRipple);


Ken had me replacing isOk with isTrue since isOk is truthy.

williamernest · 2018-03-05T21:12:14Z

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

+test('#adapter.addClassToLeadingIcon adds a class to the leading icon element', () => {
+  const {root, component} = setupTest();
+  const leadingIcon = bel`
+    <i class="material-icons mdc-chip__icon mdc-chip__icon--leading">face</i>


Does it make sense to move this to a common area since it's used in 4 tests?

williamernest · 2018-03-05T21:24:28Z

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

+  opacity: 1;
+}
+
+.mdc-chip__icon--leading + .mdc-chip__checkmark {


It seems like this should be nested in .mdc-chip__icon--leading block above.

williamernest · 2018-03-05T21:26:45Z

packages/mdc-chips/README.md

@@ -86,6 +113,8 @@ CSS Class | Description
 `mdc-chip__icon` | Optional. Indicates an icon in the chip
 `mdc-chip__icon--leading` | Optional. Indicates a leading icon in the chip
 `mdc-chip__icon--trailing` | Optional. Indicates a trailing icon in the chip
+`mdc-chip__checkmark` | Optional. Indicates the checkmark SVG element in a filter chip


Nit: Trailing period consistency. Two descriptions have trailing punctuation, but most don't.

I'm going to be annoying, but any time we have multiple sentences already, the last one should end with a period too, so technically we should have periods at the end of all of these. That can be handled separately from this PR though, at least this is consistent for now.

Haha it's not annoying. Done.

…transition end event

kfranqueiro · 2018-03-08T21:56:43Z

packages/mdc-chips/README.md

-`deregisterInteractionHandler(evtType: string, handler: EventListener) => void` | Deregisters an event listener on the root element
+`addClassToLeadingIcon(className: string) => void` | Adds a class to the leading icon element
+`removeClassFromLeadingIcon(className: string) => void` | Removes a class from the leading icon element
+`eventTargetHasClass(target: EventTarget, className: string) => boolean` | Returns true if target has className, false otherwise. 


Inconsistent period here while the others don't have one

kfranqueiro · 2018-03-08T21:57:40Z

packages/mdc-chips/_mixins.scss

@@ -61,6 +61,10 @@
      @include mdc-theme-prop(color, $color);
    }
  }
+
+  .mdc-chip__checkmark-path {
+    @include mdc-theme-prop(stroke, $color);


Followed up offline about this and realized that it kinda doesn't matter whether it were to use currentColor or not, since either way this mixin is the only place we're setting up this color.

kfranqueiro · 2018-03-08T21:58:31Z

packages/mdc-chips/chip/adapter.js

+
+  /**
+   * Returns true if target has className, false otherwise.
+   * @param {EventTarget} target


!EventTarget

kfranqueiro · 2018-03-08T21:58:53Z

packages/mdc-chips/chip/adapter.js

+   * Returns true if target has className, false otherwise.
+   * @param {EventTarget} target
+   * @param {string} className
+   */


Needs @return {boolean}

kfranqueiro · 2018-03-08T22:00:41Z

packages/mdc-chips/chip/foundation.js

-      registerInteractionHandler: () => {},
-      deregisterInteractionHandler: () => {},
+      addClassToLeadingIcon: () => {},
+      eventTargetHasClass: () => {},


Nit: move this below removeClassFromLeadingIcon to match adapter.js?

kfranqueiro · 2018-03-08T22:01:52Z

packages/mdc-chips/chip/foundation.js

+   * @param {!Event} evt
+   */
+  handleTransitionEnd_(evt) {
+    if (!(evt.propertyName === 'opacity')) {


Ease up on the parens... if (evt.propertyName !== 'opacity')

Wow major brain fart on the opposite of === 😛

kfranqueiro · 2018-03-08T22:04:28Z

packages/mdc-chips/chip/foundation.js

+        this.adapter_.hasClass(cssClasses.SELECTED)) {
+      this.adapter_.addClassToLeadingIcon(cssClasses.HIDDEN_LEADING_ICON);
+    } else if (this.adapter_.eventTargetHasClass(evt.target, cssClasses.CHECKMARK) &&
+              !this.adapter_.hasClass(cssClasses.SELECTED)) {


If you're trying to line this line up after the opening paren, this needs one more space

kfranqueiro · 2018-03-08T22:12:26Z

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

+  const mockEvt = {
+    type: 'transitionend',
+    target: {
+      classList: ['mdc-chip__checkmark'],


Is this even necessary here and in the next test, when you're mocking the adapter calls? It wasn't necessary in the previous 2 tests...

kfranqueiro · 2018-03-09T01:24:19Z

packages/mdc-chips/chip/index.js

+          this.leadingIcon_.classList.remove(className);
+        }
+      },
+      eventTargetHasClass: (target, className) => target.classList.contains(className),


Turns out classList doesn't exist on SVG elements in IE 11 :( also I don't think className behaves the same as on other DOM elements either.

Wrapped a div around the SVG to make IE 11 happy...the div now has the mdc-chip__checkmark class and the SVG has mdc-chip__checkmark-svg.

bonniezhou · 2018-03-12T17:21:28Z

Known bug on IE/Edge filed separately: #2384

This reverts commit 1e3d496.

kfranqueiro

We should add BREAKING CHANGE: to the commit message for the renamed and added adapter APIs.

bonniezhou and others added 16 commits February 21, 2018 15:08

feat(chips): Add multi-select for filter chips

51a3f28

Fix test

ee7a9b1

Hide/unhide leading and filter icons

b3d1c45

Animations

fd5a6e6

Checkmark filter icon animations working beautifully

7c5e552

svg checkmark WIP

4ad4f01

Get checkmark working with no leading icon

830c49c

Get checkmark to replace leading icon

da9c19a

Reorganize css

5803bbc

Got everything working omg

3d29476

Clean it up

13cda04

feat(chips): Replace leading icon with checkmark in selected filter c…

3bbc49f

…hips

Merge branch 'master' into feat/chips/filter-checkmark

32882eb

Clean up and add readme

0590d82

fix tests WIP

6d72ce7

update readme

08a3d72

bonniezhou mentioned this pull request Mar 2, 2018

Add optional width/height parameters to MDCRipple.layout() #2334

Closed

bonniezhou and others added 3 commits March 2, 2018 10:47

Merge branch 'master' into feat/chips/filter-checkmark

e474cd2

Adjust merge with selected color

c7fac24

Add tests

fc50fd3

bonniezhou requested review from kfranqueiro and williamernest March 2, 2018 20:07

Add tests for coverage

cb805d8

Merge branch 'master' into feat/chips/filter-checkmark

34c806f

williamernest reviewed Mar 5, 2018

View reviewed changes

bonniezhou and others added 4 commits March 5, 2018 15:18

change isOk to isTrue

65055a2

getLeadingIcon

66a9cc9

Nesting and readme update

2be956e

Merge branch 'master' into feat/chips/filter-checkmark

a6027c2

bonniezhou and others added 7 commits March 7, 2018 14:18

Reuse event handler for toplevel chip for leading icon and checkmark …

f33da26

…transition end event

Rename registerInteractionHandler to registerEventHandler

581a41f

Store leading icon in instance properties

cb19f1a

add animation and checkmark to package.json

d2c5fad

Nits

965d79e

Merge branch 'master' into feat/chips/filter-checkmark

6e447a5

Remove extra vertical-align

7beb1fd

kfranqueiro suggested changes Mar 8, 2018

View reviewed changes

bonniezhou and others added 2 commits March 8, 2018 15:23

Nits

7d04e5c

Merge branch 'master' into feat/chips/filter-checkmark

8854e40

kfranqueiro reviewed Mar 9, 2018

View reviewed changes

bonniezhou and others added 8 commits March 8, 2018 20:39

proper Closure for eventTargetHasClass

a288a0e

Wrap div around svg because IE

c90213b

Update readme

bf868ef

Merge branch 'master' into feat/chips/filter-checkmark

ff045d7

Force closure to accept evt.target

75884c5

Merge branch 'master' into feat/chips/filter-checkmark

a4d7b24

Remove all classes from svg for IE

1e3d496

Merge branch 'master' into feat/chips/filter-checkmark

44a4982

bonniezhou closed this Mar 13, 2018

bonniezhou force-pushed the feat/chips/filter-checkmark branch from a34730a to 8b799a5 Compare March 13, 2018 18:23

bonniezhou reopened this Mar 13, 2018

bonniezhou and others added 2 commits March 13, 2018 11:35

Revert "Remove all classes from svg for IE"

dff38f8

This reverts commit 1e3d496.

Merge branch 'master' into feat/chips/filter-checkmark

dfa6d26

kfranqueiro approved these changes Mar 13, 2018

View reviewed changes

Merge branch 'master' into feat/chips/filter-checkmark

1a84c1c

bonniezhou merged commit 0b73002 into master Mar 13, 2018

bonniezhou deleted the feat/chips/filter-checkmark branch March 13, 2018 22:42

dependabot-preview bot mentioned this pull request Mar 19, 2018

Update material-components-web requirement to ^0.33.0 gasolin/gasolin.github.io#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(chips): Replace leading icon with checkmark in selected filter chips #2320

feat(chips): Replace leading icon with checkmark in selected filter chips #2320

bonniezhou commented Feb 28, 2018 •

edited

Loading

codecov-io commented Mar 2, 2018 •

edited

Loading

williamernest Mar 5, 2018

williamernest Mar 5, 2018

williamernest Mar 5, 2018

williamernest Mar 5, 2018

kfranqueiro Mar 6, 2018

bonniezhou Mar 7, 2018

kfranqueiro Mar 8, 2018

kfranqueiro Mar 8, 2018

kfranqueiro Mar 8, 2018

kfranqueiro Mar 8, 2018

kfranqueiro Mar 8, 2018

kfranqueiro Mar 8, 2018

bonniezhou Mar 8, 2018

kfranqueiro Mar 8, 2018

kfranqueiro Mar 8, 2018

kfranqueiro Mar 9, 2018

bonniezhou Mar 9, 2018

bonniezhou commented Mar 12, 2018

kfranqueiro left a comment

feat(chips): Replace leading icon with checkmark in selected filter chips #2320

feat(chips): Replace leading icon with checkmark in selected filter chips #2320

Conversation

bonniezhou commented Feb 28, 2018 • edited Loading

codecov-io commented Mar 2, 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

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 Mar 12, 2018

kfranqueiro left a comment

Choose a reason for hiding this comment

bonniezhou commented Feb 28, 2018 •

edited

Loading

codecov-io commented Mar 2, 2018 •

edited

Loading