feat(chips): make deselect and toggleSelect private. Update handleChipInteraction/Removal API

[moog16]

Fixes #3612
Fixes #3613
Fixes #3615

BREAKING CHANGE: deselect and toggleSelect are private methods. handleChipInteraction and handleChipRemoval now accept chipId instead of an event.

[kfranqueiro]

I don't think this is going to fly in IE. (@williamernest can probably give you more details quickly.)

[moog16]

Yup, talked to Will - thanks for catching it. Will fix it.

[mdc-web-bot]

All 573 screenshot tests passed for commit be61243 vs. master! 💯🎉

[codecov-io]

Codecov Report

Merging #3617 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3617      +/-   ##
==========================================
+ Coverage   98.51%   98.52%   +0.01%     
==========================================
  Files         120      120              
  Lines        5239     5236       -3     
  Branches      658      658              
==========================================
- Hits         5161     5159       -2     
+ Misses         78       77       -1

Impacted Files	Coverage Δ
packages/mdc-chips/chip-set/index.js	98.24% <100%> (ø)	⬆️
packages/mdc-chips/chip-set/foundation.js	100% <100%> (+2.12%)	⬆️

---

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 313618a...06329e4. Read the comment docs.

[kfranqueiro]

2 questions:

• Given that deselect and toggleSelect are now private, what still uses select publicly?
• We still have a unit test testing deselect_ directly even though it's a private API now... should we rework that test to test the API that calls deselect_ instead?

[moog16]

If you want to initialize a chip as selected, select will need to be called.

Should we not test private methods? I can add a test that triggers the _deselect method to be thorough.

[kfranqueiro]

RE select, I see it now in the component's initialSyncWithDOM.

We typically directly test public APIs with enough cases to cover all private code. Testing private methods directly veers towards testing implementation details.

[moog16]

@kfranqueiro ... pushed changes for tests.

RE select, I think you're on to something. I think it should be private too. Instead of using it in initialSyncWithDOM, the ChipSet should instead listen to the SELECTION_EVENT. ChipSet will then not need to check for chip.selected state. If you agree, I can do the changes/update the tests and Readme.

[kfranqueiro]

I don't necessarily think the use of select is invalid... How would listening to the event work for initially-selected chips at page load based on applied classes?

[moog16]

@rlfriedman what do you think about my above suggestion? Does that work for you?

[moog16]

ChipSet will be initialized with the SELECTION_EVENT listening. The Chips will each be initialized and put into selection state: chip.selected = true, which emits the SELECTION_EVENT. Currently the ChipSet already has a listener, which looks like it already calls the foundation.handleChipSelection, which calls foundation.select (which is what the initialSyncWithDOM calls). So seems like what we have now is redundant.

[kfranqueiro]

I'm not confident that's the case, because AFAICT nothing else will ever actually invoke the Chip's selected setter when it initializes. Chip Set's initialSyncWithDOM invokes Chip's selected getter which determines its result based on the already-applied CSS class.

I'm also not sure it'd be correct to change that, since emitting events for something that arguably has already happened might seem unnatural.

[moog16]

I see...I did some more investigation and it looks like the demo only initializes the ChipSet, which also initializes all it's Chips, which is why it is needed. So nevermind. :)

[mdc-web-bot]

All 557 screenshot tests passed for commit d4da2d4 vs. master! 💯🎉

[moog16]

@kfranqueiro - is this PR good to go?

[kfranqueiro]

Yeah I think so.

[mdc-web-bot]

All 557 screenshot tests passed for commit 6ffaea8 vs. master! 💯🎉

[mdc-web-bot]

All 557 screenshot tests passed for commit 612c135 vs. master! 💯🎉

[mdc-web-bot]

All 557 screenshot tests passed for commit f287e48 vs. master! 💯🎉

[mdc-web-bot]

All 557 screenshot tests passed for commit b9438ad vs. master! 💯🎉

[mdc-web-bot]

All 557 screenshot tests passed for commit 06329e4 vs. master! 💯🎉