feat(Dropdown): add clearable prop
#885
Conversation
layershifter
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #885 +/- ##
=======================================
Coverage 92.99% 92.99%
=======================================
Files 21 21
Lines 728 728
Branches 69 69
=======================================
Hits 677 677
Misses 51 51Continue to review full report at Codecov.
|
packages/react/src/themes/teams/components/Dropdown/dropdownStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/Dropdown/dropdownStyles.ts
Outdated
Show resolved
Hide resolved
docs/src/examples/components/Dropdown/Types/DropdownExampleClearable.shorthand.steps.ts
Show resolved
Hide resolved
…ub.com/stardust-ui/react into feat/dropdown-clearable
| @@ -17,6 +17,14 @@ Test a feature | |||
| - [Important mentions:](#important-mentions) | |||
| - [Run Screener tests](#run-screener-tests) | |||
| - [Local run command](#local-run-command) | |||
| - [Behavior tets](#behavior-tets) | |||
There was a problem hiding this comment.
Why are these changes here? Bad merge?
There was a problem hiding this comment.
It wasn't generated before 🤔
| @@ -86,7 +95,7 @@ export interface DropdownProps extends UIComponentProps<DropdownProps, DropdownS | |||
| inline?: boolean | |||
|
|
|||
| /** Array of props for generating list options (Dropdown.Item[]) and selected item labels(Dropdown.SelectedItem[]), if it's a multiple selection. */ | |||
| items?: ShorthandValue[] | |||
| items?: ShorthandCollection | |||
There was a problem hiding this comment.
Never mind I was reading the changes mixed. Good change! 👍
| @@ -192,6 +201,8 @@ class Dropdown extends AutoControlledComponent<Extendable<DropdownProps>, Dropdo | |||
| content: false, | |||
| }), | |||
| activeSelectedIndex: PropTypes.number, | |||
| clearable: PropTypes.bool, | |||
There was a problem hiding this comment.
I don't like that we have two new props in the API, but I don't have any better proposal for now...
| @@ -226,6 +237,7 @@ class Dropdown extends AutoControlledComponent<Extendable<DropdownProps>, Dropdo | |||
|
|
|||
| static defaultProps: DropdownProps = { | |||
| as: 'div', | |||
| clearIndicator: 'close', | |||
There was a problem hiding this comment.
Please, don't assume that some icon exists, as in some theme they may not. That's the reason we introduced the Indicator component, instead of using the chevron icons inside the components. Can we use here some unicode char by default if no icon i provided?
There was a problem hiding this comment.
Input component does the same thing actually. We can refactor them separately later.
The main issue with unicode chars, that I was not able to find a good symbol. Do you have a proposal about it?
There was a problem hiding this comment.
I am aware that the Input does the same thing, agreed to tackle this in separate PR. This brings me back to the fact that maybe we should have some icons in the base theme, at least for the things we need in the components (close, arrows etc..) Let's create separate issue for this.
| ? Icon.create(clearIndicator, { | ||
| defaultProps: { | ||
| className: Dropdown.slotClassNames.clearIndicator, | ||
| onClick: this.handleClear, |
There was a problem hiding this comment.
Shouldn't onClick be on the override props? Then you can do all necessary for the Dropdown, and then invoke the user's onClick if provided.
| ] | ||
|
|
||
| const DropdownClearableExample = () => ( | ||
| <Dropdown clearable items={inputItems} placeholder="Select your hero" /> |
There was a problem hiding this comment.
Maybe we should add additional example for showing customization of the clearableIndicator?
There was a problem hiding this comment.
If we will have request for it, we can introduce it. For now, I am not sure that we need to add example for an every slot, it can make docs unusable ⛸
Refs #560.
Why to separate props?
clearablesets the behavior, whileclearIndicatorallows to customize the slot.