fix(chips): Manage chip selection for classes added manually #2391
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2391 +/- ##
==========================================
+ Coverage 98.85% 98.85% +<.01%
==========================================
Files 103 103
Lines 4111 4121 +10
Branches 516 516
==========================================
+ Hits 4064 4074 +10
Misses 47 47
Continue to review full report at Codecov.
|
packages/mdc-chips/README.md
Outdated
|
|
||
| Method Signature | Description | ||
| --- | --- | ||
| `manageSelection() => void` | Manages the selection state of the chip set, given a chip that was just selected/deselected. |
| * @param {!Object} evt | ||
| * @private | ||
| * Manages the selection state of the chip set, given a chip that was just selected/deselected. | ||
| * @param {!MDCChip} chip |
There was a problem hiding this comment.
Will this param type be a problem for framework wrappers?
There was a problem hiding this comment.
This is a really good point, meaning that selectedChips_ should also not hold MDCChip objects.
How Text Field gets around this is having the main component hold sub-components, and pass sub-foundations to the main foundation. For example, MDCTextField has MDCTextFieldHelperText and passes MDCTextFieldHelperTextFoundation to MDCTextFieldFoundation. What do you think about doing the same here, so MDCChipSetFoundation would control MDCChipFoundation objects?
I can create a separate PR and punt this one until that gets merged.
|
|
||
| /** | ||
| * Handles a chip interaction event | ||
| * @param {!Object} evt |
| @@ -58,6 +58,14 @@ class MDCChipSet extends MDCComponent { | |||
| }); | |||
| } | |||
|
|
|||
| initialSyncWithDOM() { | |||
There was a problem hiding this comment.
We might want to add a test for this to prevent regressions
| */ | ||
| handleChipInteraction_(evt) { | ||
| const {chip} = evt.detail; | ||
| manageSelection(chip) { |
There was a problem hiding this comment.
I'm not wild about this method name, but I can't really think of a better alternative lol.
Maybe that's a signal that the method itself is trying to do too much, and needs to be refactored?
I also feel like in general, methods that "toggle" things tend to be error-prone and hard to use, because you always need to know the previous state before you call them.
It might be clearer and simpler to have two separate methods that are idempotent:
select(chip) {
if (this.isChoice_()) {
this.deselectAll_();
}
chip.setSelected(true);
this.selectedChips_.push(chip);
}
deselect(chip) {
const index = this.selectedChips_.indexOf(chip);
if (index >= 0) {
this.selectedChips_.splice(index, 1);
}
chip.setSelected(false);
}
...
isChoice_() {
return this.adapter_.hasClass(cssClasses.CHOICE);
}
deselectAll_() {
this.selectedChips_.forEach((chip) => {
chip.setSelected(false);
});
this.selectedChips_.length = 0;
}And the interaction handler can decide which method to call:
handleChipInteraction_(evt) {
const {chip} = evt.detail;
if (chip.isSelected()) {
this.deselect(chip);
} else {
this.select(chip);
}
}WDYT?
There was a problem hiding this comment.
Meant to respond here earlier...
This looks great! This will also make it easier to collapse chip and chip-set into one controller, if we ever decide to do that.
| @@ -44,6 +44,7 @@ test('attachTo returns an MDCChipSet instance', () => { | |||
| class FakeChip { | |||
There was a problem hiding this comment.
It might be good to update the demo page so that "Choice" and "Filter" chips have an initial selection.
packages/mdc-chips/README.md
Outdated
| </div> | ||
| ``` | ||
|
|
||
| To pre-select filter chips that have a leading icon, also add the class `mdc-chip__icon--hidden-leading` to the `mdc-chip__icon--leading` element. This will ensure that the checkmark displaces the leading icon. |
There was a problem hiding this comment.
Nit: I think our convention has generally been mdc-chip__icon--leading-hidden (rather than hidden-leading)
packages/mdc-chips/README.md
Outdated
| ```html | ||
| <div class="mdc-chip mdc-chip--selected"> | ||
| <i class="material-icons mdc-chip__icon mdc-chip__icon--leading mdc-chip__icon--hidden-leading">face</i> | ||
| <div class="mdc-chip__checkmark" > |
There was a problem hiding this comment.
Nit: Remove space before >
| */ | ||
| manageSelection(chip) { | ||
| select(chipFoundation) { |
There was a problem hiding this comment.
This refactored code is beautiful. Nice work!! 😀
Fixes #2374.
Note that if the
mdc-chip--selectedclass was added to more than one choice chip, only the last one will be selected.