Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(chips): Handle multi-select for filter chips #2297

Merged
merged 8 commits into from
Feb 27, 2018
Merged

Conversation

bonniezhou
Copy link
Contributor

Add multiple selection behavior for filter chips.

Follow-up PRs will address #2295 and #2296.

@codecov-io
Copy link

codecov-io commented Feb 23, 2018

Codecov Report

Merging #2297 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2297      +/-   ##
==========================================
+ Coverage   99.04%   99.07%   +0.02%     
==========================================
  Files          99       99              
  Lines        3999     4004       +5     
  Branches      513      515       +2     
==========================================
+ Hits         3961     3967       +6     
+ Misses         38       37       -1
Impacted Files Coverage Δ
packages/mdc-chips/chip-set/constants.js 100% <ø> (ø) ⬆️
packages/mdc-chips/chip-set/foundation.js 100% <100%> (+3.57%) ⬆️

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 d28db49...35badc6. Read the comment docs.

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, but...are filter chips supposed to be removable eventually? We might have to handle that carefully to make sure stale chips don't end up in the activeChips array...

@bonniezhou
Copy link
Contributor Author

Yep, we also have to make sure that stale chips are removed from the chip-set's chips array.

@kfranqueiro I'm maybe rethinking the design of this... what do you think about adding a selected field to each chip and a selectedChoiceChip field to the chip-set, rather than having the chip-set hold a selectedChips array? This means that querying the selected chips would require looping through the entire chip-set's chips array, but the flip side is that it saves memory.
Though I'm not sure how much time/memory either option is really saving, so I think it comes down to what is best practice?

@bonniezhou bonniezhou merged commit 807b6ce into master Feb 27, 2018
@bonniezhou bonniezhou deleted the feat/chips/filter branch February 27, 2018 16:37
@kfranqueiro
Copy link
Contributor

We discussed offline and decided to stick with the selectedChips array, figuring that the number of total chips is more likely to far outnumber the number of selected chips to the point where this may be ultimately more efficient. Moreover, having the separate array may be helpful for eventually providing information on selected chips in events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants