Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(dropdown): multiple selection #845

Merged
merged 9 commits into from
Feb 6, 2019
Merged

feat(dropdown): multiple selection #845

merged 9 commits into from
Feb 6, 2019

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented Feb 5, 2019

feat(dropdown): multiple selection

Description:

This PR implements the multiple selection with trigger button flavor for Dropdown component (easy to leverage existing functionality)

API

<Dropdown
  multiple
  items={inputItems}
  placeholder="Start typing a name"
  noResultsMessage="We couldn't find any matches."
/>

renders:
screen recording 2019-02-04 at 21 44 10

mnajdova
mnajdova reviewed Feb 5, 2019
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@mnajdova mnajdova mentioned this pull request Feb 5, 2019
Merged
@bmdalex bmdalex force-pushed the feat/dropdown-multiple-selection branch from a52bbea to 472c222 Compare February 6, 2019 11:00
@codecov
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

Merging #845 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #845   +/-   ##
=======================================
  Coverage   93.54%   93.54%           
=======================================
  Files          21       21           
  Lines         728      728           
  Branches       69       73    +4     
=======================================
  Hits          681      681           
  Misses         47       47

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 6a049c1...472c222. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 6, 2019 •
edited

Codecov Report

Merging #845 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #845   +/-   ##
=======================================
  Coverage   93.54%   93.54%           
=======================================
  Files          21       21           
  Lines         728      728           
  Branches       73       73           
=======================================
  Hits          681      681           
  Misses         47       47

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 d89668f...d38f328. Read the comment docs.

@bmdalex bmdalex force-pushed the feat/dropdown-multiple-selection branch 2 times, most recently from b53d0d9 to 4701332 Compare February 6, 2019 13:59
silviuaavram
silviuaavram approved these changes Feb 6, 2019
View reviewed changes
docs/src/examples/components/Dropdown/Types/DropdownExampleMultipleSelection.shorthand.steps.ts Outdated Show resolved Hide resolved
packages/react/src/components/Dropdown/Dropdown.tsx Outdated
* otherwise, for single selection we convert the value with itemToString
* and for multiple selection we return empty string, the values are rendered by renderSelectedItems
*/
private getSelectedItemAsString = (value: ShorthandValue): string => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need that parameter if we can get it from state as we did with the props? this method might as well be getTriggerButtonMessage or getTriggerButtonText. not important but was just curious why not like that. oh, and capital I in If there is no value...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this method is also used here, but not merged yet:
https://github.com/stardust-ui/react/pull/839/files#r254290634

with a different parameter, so the proposed name would not apply anymore, open to other naming suggestions

Will add capital "I", thanks

@bmdalex bmdalex force-pushed the feat/dropdown-multiple-selection branch from 4701332 to 581688e Compare February 6, 2019 14:49
@bmdalex bmdalex force-pushed the feat/dropdown-multiple-selection branch from 581688e to 604ccd5 Compare February 6, 2019 15:46
@bmdalex bmdalex merged commit 9559996 into master Feb 6, 2019
@bmdalex bmdalex deleted the feat/dropdown-multiple-selection branch February 7, 2019 14:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mnajdova mnajdova mnajdova approved these changes

@kuzhelov kuzhelov kuzhelov approved these changes

@silviuaavram silviuaavram silviuaavram approved these changes

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@layershifter layershifter Awaiting requested review from layershifter layershifter is a code owner

@levithomason levithomason Awaiting requested review from levithomason levithomason is a code owner

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny miroslavstastny is a code owner

Assignees
No one assigned
Labels
ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bmdalex @mnajdova @kuzhelov @silviuaavram