feat: implement showCheckbox prop on the MultiSelect component
#1689
Conversation
…to implement-showCheckBox-prop
Features
Bug Fixes
Contributors@HellWolf93, @TahimiLeonBravo, @LeandroTorresSicilia Commit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
HellWolf93
requested review from
LeandroTorresSicilia and
TahimiLeonBravo
and removed request for
LeandroTorresSicilia
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
…to implement-showCheckBox-prop
…Wolf93/react-rainbow into implement-showCheckBox-prop
| @@ -207,10 +224,17 @@ const InternalDropdown = forwardRef((props, reference) => { | |||
| return handleChange(rest); | |||
| }; | |||
|
|
|||
| const handleTabKeyPressed = event => { | |||
| if (showCheckbox && event.target.tagName !== 'BUTTON') { | |||
| event.stopPropagation(); | |||
There was a problem hiding this comment.
we must avoid doing stopPropagation take a look at what we did in Tree component (src/components/Tree/helpers/shouldSelectNode.js), instead of calling stopPropagation we did some kind of filter
What happens is that if someone use this component and in its implementation, he uses a global listener then it will not get this event, we must avoid this behavior.
I know we use it in other place and we need to fix them, but for now what we can do is no doing it anymore
| value: childValue, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
here we should take a more functional approach like using map or reduce
| return ( | ||
| <StyledHeader title={title} role="presentation" onMouseDown={onClick}> | ||
| <StyledPrimitiveCheckbox type="checkbox" label="" checked={isChecked} {...rest} /> | ||
| <StyledHeaderLabel>{label}</StyledHeaderLabel> |
There was a problem hiding this comment.
I see there are two like this, other in the InternalDropdown, both are needed?
There was a problem hiding this comment.
Nop, just a leftover when moving the 'Select all' logic to the InternalDropdown component. Done.
src/components/Option/index.js
Outdated
| const { disabled, privateOnClick, label, name, icon, value, showCheckbox } = this.props; | ||
| if (showCheckbox) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); |
There was a problem hiding this comment.
same ^^ we need to avoid stopPropagation
src/components/Option/index.js
Outdated
| const { disabled, privateOnClick, label, name, icon, value } = this.props; | ||
| event.preventDefault(); | ||
| event.stopPropagation(); |
|
@HellWolf93 there are some tests failing |
…Wolf93/react-rainbow into implement-showCheckBox-prop
|
when press the only button when some or all items are selected, included the pressed one,first it uncheck the same item that the only was pressed and then it trigger the only function, then it send to events, one removing the item and then setting this item only |
|
When selecting all it does not send the icons, we need to add test to this in order to guarantee this functionality |
|
There unit test failing |
…Wolf93/react-rainbow into implement-showCheckBox-prop
|
Code Climate has analyzed commit ed8b28d and detected 11 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
fix: #1655
Changes proposed in this PR:
Add showCheckbox prop to MultiSelect component
I have followed (at least) the PR section of the contributing guide.