feat(chips): Make chip exit on trailing icon click optional #2893
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2893 +/- ##
=========================================
+ Coverage 98.3% 98.3% +<.01%
=========================================
Files 101 101
Lines 4363 4375 +12
Branches 563 564 +1
=========================================
+ Hits 4289 4301 +12
Misses 74 74
Continue to review full report at Codecov.
|
packages/mdc-chips/README.md
Outdated
| @@ -276,6 +277,8 @@ Method Signature | Description | |||
| --- | --- | |||
| `isSelected() => boolean` | Returns true if the chip is selected | |||
| `setSelected(selected: boolean) => void` | Sets the chip's selected state | |||
| `shouldRemoveOnTrailingIconClick() => boolean` | Returns whether a trailing icon click should trigger exit/removal of the chip | |||
There was a problem hiding this comment.
should this be getShouldRemove... to complement setShouldRemove...?
demos/chips.html
Outdated
| function resetInputChipSet() { | ||
| inputChipSetComponent.destroy(); | ||
| if (this.checked) { | ||
| inputChipSetComponent = new mdc.chips.MDCChipSet(inputChipSetEl, undefined, (chipEl) => { |
There was a problem hiding this comment.
Use function, not => (IE 11)
There was a problem hiding this comment.
argh, nice catch
packages/mdc-chips/README.md
Outdated
| `isSelected() => boolean` | Proxies to the foundation's `isSelected` method | ||
| `beginExit() => void` | Begins the exit animation which leads to removal of the chip | ||
|
|
||
| Property | Value Type | Description | ||
| --- | --- | --- | ||
| `foundation` | MDCChipFoundation | The foundation | ||
| `shouldRemoveOnTrailingIconClick` | Boolean | Proxies to the foundation's `shouldRemoveOnTrailingIconClick`/`setShouldRemoveOnTrailingIconClick` methods |
There was a problem hiding this comment.
Need to add get to the foundation method name in the description.
Also, an additional thought: add a sentence here explaining that if this is set to false, manually call beginExit on the chip component to control when to remove it.
There was a problem hiding this comment.
Sounds good, added as a NOTE to stick with having one line per table entry
| this.adapter_.addClass(cssClasses.CHIP_EXIT); | ||
| if (this.shouldRemoveOnTrailingIconClick_) { | ||
| // Begins the exit animation which leads to removal of the chip | ||
| this.adapter_.addClass(cssClasses.CHIP_EXIT); |
There was a problem hiding this comment.
It occurs to me that this is effectively doing the same thing that beginExit on the component is doing. Do you think we should create a beginExit foundation method and proxy to it from the component's method, so that both can reuse the same code?
There was a problem hiding this comment.
I thought about that but realized that's actually more code?
...But I guess it makes sense pattern-wise. Ok I'll update.
There was a problem hiding this comment.
It probably is more code ultimately, but in this case the trade-off is avoiding repeating the same logic across modules which risks skew later.
packages/mdc-chips/README.md
Outdated
| `ripple` | `MDCRipple` | The `MDCRipple` instance for the root element that `MDCChip` initializes | ||
| >_NOTE_: If `shouldRemoveOnTrailingIconClick` is set to false, you must manually call `beginExit()` on the chip to remove it. |
There was a problem hiding this comment.
Add empty line between table and blockquote
|
@bonniezhou Offering name suggestion, |
|
I think that could be misinterpreted as whether you can remove the chip at all, not whether it will be removed automatically upon click. :( |
Fixes #2869.
Taking name suggestions to shorten
shouldRemoveOnTrailingIconClick....