From 07078bbb62d9f598461c7ff3c6b51c67d7b339de Mon Sep 17 00:00:00 2001 From: Patty RoDee Date: Tue, 2 Jul 2019 09:43:26 -0700 Subject: [PATCH] feat(chips): Use index for all chip operations (#4869) Change interface for MDCChipSetAdapter#removeChip to MDCChipSetAdapter#removeChipAtIndex. Update comments for MDCChipSetAdapter. BREAKING CHANGE: MDCChipSetAdapter#removeChip has been replaced with MDCChipSetAdapter#removeChipAtIndex. MDCChipSetAdapter#setSelected has been replaced with MDCChipSetAdapter#selectChipAtIndex --- packages/mdc-chips/README.md | 4 +- packages/mdc-chips/chip-set/adapter.ts | 18 +++++-- packages/mdc-chips/chip-set/component.ts | 10 ++-- packages/mdc-chips/chip-set/foundation.ts | 15 +++--- .../mdc-chips/mdc-chip-set.foundation.test.js | 47 ++++++++++--------- test/unit/mdc-chips/mdc-chip-set.test.js | 12 ++--- 6 files changed, 59 insertions(+), 47 deletions(-) diff --git a/packages/mdc-chips/README.md b/packages/mdc-chips/README.md index 448a40c916e..d934c103f5b 100644 --- a/packages/mdc-chips/README.md +++ b/packages/mdc-chips/README.md @@ -391,8 +391,8 @@ Method Signature | Description Method Signature | Description --- | --- `hasClass(className: string) => boolean` | Returns whether the chip set element has the given class -`removeChip(chipId: string) => void` | Removes the chip with the given id from the chip set -`setSelected(chipId: string, selected: boolean) => void` | Sets the selected state of the chip with the given id +`removeChipAtIndex(index: number) => void` | Removes the chip with the given `index` from the chip set +`selectChipAtIndex(index: string, selected: boolean) => void` | Sets the selected state of the chip at the given `index` `getIndexOfChipById(id: string) => number` | Returns the index of the chip with the matching `id` or -1 `focusChipPrimaryActionAtIndex(index: number) => void` | Calls `MDCChip#focusPrimaryAction()` on the chip at the given `index` `focusChipTrailingActionAtIndex(index: number) => void` | Calls `MDCChip#focusTrailingAction()` on the chip at the given `index` diff --git a/packages/mdc-chips/chip-set/adapter.ts b/packages/mdc-chips/chip-set/adapter.ts index 56ad29f4161..05a92e2523a 100644 --- a/packages/mdc-chips/chip-set/adapter.ts +++ b/packages/mdc-chips/chip-set/adapter.ts @@ -35,23 +35,33 @@ export interface MDCChipSetAdapter { hasClass(className: string): boolean; /** - * Removes the chip with the given id from the chip set. + * Removes the chip with the given index from the chip set. + * Make sure to remove it from the chip list, too. */ - removeChip(chipId: string): void; + removeChipAtIndex(index: number): void; /** - * Sets the selected state of the chip with the given id. + * Sets the selected state of the chip at the given index. */ - setSelected(chipId: string, selected: boolean): void; + selectChipAtIndex(index: number, selected: boolean): void; /** + * Returns the index of the chip at the given ID. * @param chipId the unique ID of the chip * @return the numerical index of the chip with the matching id or -1. */ getIndexOfChipById(chipId: string): number; + /** + * Calls Chip#focusPrimaryAction() on the chip at the given index. + * @param index the index of the chip + */ focusChipPrimaryActionAtIndex(index: number): void; + /** + * Calls Chip#focusTrailingAction() on the chip at the given index. + * @param index the index of the chip + */ focusChipTrailingActionAtIndex(index: number): void; /** diff --git a/packages/mdc-chips/chip-set/component.ts b/packages/mdc-chips/chip-set/component.ts index f56aebe5afc..ea7b88ae047 100644 --- a/packages/mdc-chips/chip-set/component.ts +++ b/packages/mdc-chips/chip-set/component.ts @@ -120,9 +120,8 @@ export class MDCChipSet extends MDCComponent { }, hasClass: (className) => this.root_.classList.contains(className), isRTL: () => window.getComputedStyle(this.root_).getPropertyValue('direction') === 'rtl', - removeChip: (chipId) => { - const index = this.findChipIndex_(chipId); - if (index >= 0) { + removeChipAtIndex: (index) => { + if (index >= 0 && index < this.chips_.length) { this.chips_[index].destroy(); this.chips_[index].remove(); this.chips_.splice(index, 1); @@ -131,9 +130,8 @@ export class MDCChipSet extends MDCComponent { removeFocusFromChipAtIndex: (index) => { this.chips_[index].removeFocus(); }, - setSelected: (chipId, selected) => { - const index = this.findChipIndex_(chipId); - if (index >= 0) { + selectChipAtIndex: (index, selected) => { + if (index >= 0 && index < this.chips_.length) { this.chips_[index].selected = selected; } }, diff --git a/packages/mdc-chips/chip-set/foundation.ts b/packages/mdc-chips/chip-set/foundation.ts index 291e704e086..015b22351bb 100644 --- a/packages/mdc-chips/chip-set/foundation.ts +++ b/packages/mdc-chips/chip-set/foundation.ts @@ -43,9 +43,9 @@ export class MDCChipSetFoundation extends MDCFoundation { getIndexOfChipById: () => -1, hasClass: () => false, isRTL: () => false, - removeChip: () => undefined, + removeChipAtIndex: () => undefined, removeFocusFromChipAtIndex: () => undefined, - setSelected: () => undefined, + selectChipAtIndex: () => undefined, }; } @@ -75,11 +75,13 @@ export class MDCChipSetFoundation extends MDCFoundation { if (this.adapter_.hasClass(cssClasses.CHOICE) && this.selectedChipIds_.length > 0) { const previouslySelectedChip = this.selectedChipIds_[0]; + const previouslySelectedIndex = this.adapter_.getIndexOfChipById(previouslySelectedChip); this.selectedChipIds_.length = 0; - this.adapter_.setSelected(previouslySelectedChip, false); + this.adapter_.selectChipAtIndex(previouslySelectedIndex, false); } this.selectedChipIds_.push(chipId); - this.adapter_.setSelected(chipId, true); + const index = this.adapter_.getIndexOfChipById(chipId); + this.adapter_.selectChipAtIndex(index, true); } /** @@ -111,7 +113,7 @@ export class MDCChipSetFoundation extends MDCFoundation { handleChipRemoval(chipId: string) { const index = this.adapter_.getIndexOfChipById(chipId); this.deselect_(chipId); - this.adapter_.removeChip(chipId); + this.adapter_.removeChipAtIndex(index); const maxIndex = this.adapter_.getChipListCount() - 1; const nextIndex = Math.min(index, maxIndex); this.removeFocusFromChipsExcept_(nextIndex); @@ -191,7 +193,8 @@ export class MDCChipSetFoundation extends MDCFoundation { const index = this.selectedChipIds_.indexOf(chipId); if (index >= 0) { this.selectedChipIds_.splice(index, 1); - this.adapter_.setSelected(chipId, false); + const chipIndex = this.adapter_.getIndexOfChipById(chipId); + this.adapter_.selectChipAtIndex(chipIndex, false); } } diff --git a/test/unit/mdc-chips/mdc-chip-set.foundation.test.js b/test/unit/mdc-chips/mdc-chip-set.foundation.test.js index 2bb54e179c3..98dcdfff75d 100644 --- a/test/unit/mdc-chips/mdc-chip-set.foundation.test.js +++ b/test/unit/mdc-chips/mdc-chip-set.foundation.test.js @@ -42,7 +42,7 @@ test('exports cssClasses', () => { test('defaultAdapter returns a complete adapter implementation', () => { verifyDefaultAdapter(MDCChipSetFoundation, [ - 'hasClass', 'removeChip', 'setSelected', + 'hasClass', 'removeChipAtIndex', 'selectChipAtIndex', 'focusChipPrimaryActionAtIndex', 'focusChipTrailingActionAtIndex', 'getIndexOfChipById', 'isRTL', 'getChipListCount', 'removeFocusFromChipAtIndex', @@ -56,92 +56,92 @@ const setupTest = () => { }; test('in choice chips, #select does nothing if chip is already selected', () => { - const {foundation, mockAdapter} = setupTest(); + const {foundation, mockAdapter} = setupChipNavigationTest(['chipA', 'chipB']); td.when(mockAdapter.hasClass(cssClasses.CHOICE)).thenReturn(true); foundation.select('chipA'); foundation.select('chipA'); - td.verify(mockAdapter.setSelected('chipA', true), {times: 1}); + td.verify(mockAdapter.selectChipAtIndex(0, true), {times: 1}); assert.equal(foundation.getSelectedChipIds().length, 1); }); test('in choice chips, #select selects chip if no chips are selected', () => { - const {foundation, mockAdapter} = setupTest(); + const {foundation, mockAdapter} = setupChipNavigationTest(['chipA', 'chipB']); td.when(mockAdapter.hasClass(cssClasses.CHOICE)).thenReturn(true); assert.equal(foundation.getSelectedChipIds().length, 0); foundation.select('chipA'); - td.verify(mockAdapter.setSelected('chipA', true)); + td.verify(mockAdapter.selectChipAtIndex(0, true)); assert.equal(foundation.getSelectedChipIds().length, 1); }); test('in choice chips, #select deselects chip if another chip is selected', () => { - const {foundation, mockAdapter} = setupTest(); + const {foundation, mockAdapter} = setupChipNavigationTest(['chipA', 'chipB']); td.when(mockAdapter.hasClass(cssClasses.CHOICE)).thenReturn(true); foundation.select('chipB'); assert.equal(foundation.getSelectedChipIds().length, 1); foundation.select('chipA'); - td.verify(mockAdapter.setSelected('chipB', false)); - td.verify(mockAdapter.setSelected('chipA', true)); + td.verify(mockAdapter.selectChipAtIndex(1, false)); + td.verify(mockAdapter.selectChipAtIndex(0, true)); assert.equal(foundation.getSelectedChipIds().length, 1); }); test('in filter chips, #select selects multiple chips', () => { - const {foundation, mockAdapter} = setupTest(); + const {foundation, mockAdapter} = setupChipNavigationTest(['chipA', 'chipB']); td.when(mockAdapter.hasClass(cssClasses.FILTER)).thenReturn(true); assert.equal(foundation.getSelectedChipIds().length, 0); foundation.select('chipA'); - td.verify(mockAdapter.setSelected('chipA', true)); + td.verify(mockAdapter.selectChipAtIndex(0, true)); assert.equal(foundation.getSelectedChipIds().length, 1); foundation.select('chipB'); - td.verify(mockAdapter.setSelected('chipB', true)); + td.verify(mockAdapter.selectChipAtIndex(1, true)); assert.equal(foundation.getSelectedChipIds().length, 2); }); test('in filter chips, #select does nothing if chip is already selected', () => { - const {foundation, mockAdapter} = setupTest(); + const {foundation, mockAdapter} = setupChipNavigationTest(['chipA', 'chipB']); td.when(mockAdapter.hasClass(cssClasses.CHOICE)).thenReturn(false); td.when(mockAdapter.hasClass(cssClasses.FILTER)).thenReturn(true); foundation.select('chipA'); foundation.select('chipA'); - td.verify(mockAdapter.setSelected('chipA', true), {times: 1}); + td.verify(mockAdapter.selectChipAtIndex(0, true), {times: 1}); assert.equal(foundation.getSelectedChipIds().length, 1); }); test('in filter chips, #handleChipInteraction deselects chip if in selectedChipId', () => { - const {foundation, mockAdapter} = setupTest(); + const {foundation, mockAdapter} = setupChipNavigationTest(['chipA', 'chipB']); td.when(mockAdapter.hasClass(cssClasses.FILTER)).thenReturn(true); foundation.handleChipInteraction('chipA'); foundation.handleChipInteraction('chipB'); assert.equal(foundation.getSelectedChipIds().length, 2); foundation.handleChipInteraction('chipB'); - td.verify(mockAdapter.setSelected('chipB', false)); + td.verify(mockAdapter.selectChipAtIndex(1, false)); assert.equal(foundation.getSelectedChipIds().length, 1); foundation.handleChipInteraction('chipA'); - td.verify(mockAdapter.setSelected('chipA', false)); + td.verify(mockAdapter.selectChipAtIndex(0, false)); assert.equal(foundation.getSelectedChipIds().length, 0); }); test('#handleChipInteraction selects chip if the chip set is a filter chip set', () => { - const {foundation, mockAdapter} = setupTest(); + const {foundation, mockAdapter} = setupChipNavigationTest(['chipA', 'chipB']); td.when(mockAdapter.hasClass(cssClasses.CHOICE)).thenReturn(false); td.when(mockAdapter.hasClass(cssClasses.FILTER)).thenReturn(true); foundation.handleChipInteraction('chipA'); - td.verify(mockAdapter.setSelected('chipA', true)); + td.verify(mockAdapter.selectChipAtIndex(0, true)); }); test('#handleChipInteraction selects chip if the chip set is a choice chip set', () => { - const {foundation, mockAdapter} = setupTest(); + const {foundation, mockAdapter} = setupChipNavigationTest(['chipA', 'chipB']); td.when(mockAdapter.hasClass(cssClasses.CHOICE)).thenReturn(true); td.when(mockAdapter.hasClass(cssClasses.FILTER)).thenReturn(false); foundation.handleChipInteraction('chipA'); - td.verify(mockAdapter.setSelected('chipA', true)); + td.verify(mockAdapter.selectChipAtIndex(0, true)); }); test('#handleChipInteraction removes focus from all chips except the selected one', () => { @@ -156,12 +156,12 @@ test('#handleChipInteraction removes focus from all chips except the selected on }); test('#handleChipInteraction does nothing if the chip set is neither choice nor filter', () => { - const {foundation, mockAdapter} = setupTest(); + const {foundation, mockAdapter} = setupChipNavigationTest(['chipA', 'chipB', 'chipC']); td.when(mockAdapter.hasClass(cssClasses.CHOICE)).thenReturn(false); td.when(mockAdapter.hasClass(cssClasses.FILTER)).thenReturn(false); foundation.handleChipInteraction('chipA'); - td.verify(mockAdapter.setSelected('chipA', true), {times: 0}); + td.verify(mockAdapter.selectChipAtIndex(0, true), {times: 0}); }); test('#handleChipSelection selects an unselected chip if selected is true', () => { @@ -200,9 +200,10 @@ test('#handleChipSelection does nothing if selected is false and the chip is not test('#handleChipRemoval removes chip', () => { const {foundation, mockAdapter} = setupTest(); + td.when(mockAdapter.getIndexOfChipById('chipA')).thenReturn(1); foundation.handleChipRemoval('chipA'); - td.verify(mockAdapter.removeChip('chipA')); + td.verify(mockAdapter.removeChipAtIndex(1)); }); test('#handleChipRemoval removes focus from all chips except the next one', () => { diff --git a/test/unit/mdc-chips/mdc-chip-set.test.js b/test/unit/mdc-chips/mdc-chip-set.test.js index 35d09f666ee..dc0467a5457 100644 --- a/test/unit/mdc-chips/mdc-chip-set.test.js +++ b/test/unit/mdc-chips/mdc-chip-set.test.js @@ -162,25 +162,25 @@ test('#adapter.hasClass returns true if class is set on chip set element', () => assert.isTrue(component.getDefaultFoundation().adapter_.hasClass('foo')); }); -test('#adapter.removeChip removes the chip object from the chip set', () => { +test('#adapter.removeChipAtIndex removes the chip object from the chip set', () => { const {component} = setupTest(); const chip = component.chips[0]; - component.getDefaultFoundation().adapter_.removeChip(chip.id); + component.getDefaultFoundation().adapter_.removeChipAtIndex(0); assert.equal(component.chips.length, 2); td.verify(chip.destroy()); td.verify(chip.remove()); }); -test('#adapter.removeChip does nothing if the given object is not in the chip set', () => { +test('#adapter.removeChipAtIndex does nothing if the given object is not in the chip set', () => { const {component} = setupTest(); - component.getDefaultFoundation().adapter_.removeChip('chip0'); + component.getDefaultFoundation().adapter_.removeChipAtIndex(-1); assert.equal(component.chips.length, 3); }); -test('#adapter.setSelected sets selected on chip object', () => { +test('#adapter.selectChipAtIndex sets selected on chip object', () => { const {component} = setupTest(); const chip = component.chips[0]; - component.getDefaultFoundation().adapter_.setSelected(chip.id, true); + component.getDefaultFoundation().adapter_.selectChipAtIndex(0, true); assert.equal(chip.selected, true); });