-
-
Notifications
You must be signed in to change notification settings - Fork 114
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: implement MultiSelect component #1606
feat: implement MultiSelect component #1606
Conversation
Features
Bug Fixes
Contributors@HellWolf93, @TahimiLeonBravo, @LeandroTorresSicilia Commit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
src/components/MultiSelect/index.js
Outdated
return ( | ||
<StyledContainer className={className} style={style}> | ||
<Label label={label} hideLabel={hideLabel} required={required} inputId={inputId} /> | ||
<StyledInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember we have to follow the accessibility standards and since this component will be used on forms it needs to be an inputable.
Also, It needs to work with redux-form.
We need to review the w3c accessibility standards here in order to use the right HTML elements and attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TahimiLeonBravo I have a question about the operation. This component will allow the input to be a filter and when the keyprees delete key the options are deleted
…to implement-multiselect-component
const [startListeningOutsideClick, stopListeningOutsideClick] = useOutsideClick( | ||
dropdownRef, | ||
handleOutsideClick, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LeandroTorresSicilia @TahimiLeonBravo I think we should fix this hook with Rey's suggestion, before we continue using it.
src/components/MultiSelect/index.js
Outdated
return ( | ||
<StyledContainer className={className} style={style}> | ||
<Label label={label} hideLabel={hideLabel} required={required} inputId={inputId} /> | ||
<StyledInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TahimiLeonBravo I have a question about the operation. This component will allow the input to be a filter and when the keyprees delete key the options are deleted
…to implement-multiselect-component
…HellWolf93/react-rainbow into implement-multiselect-component
@@ -73,6 +73,7 @@ const InternalDropdown = forwardRef((props, reference) => { | |||
} | |||
return containerRef.current.focus(); | |||
}, | |||
contains: element => containerRef.current.contains(element), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember we need to be careful in what we expose here.
In this case why we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed in useOutsideClick hook, otherwise throws the error 'ref.current.contains' is not a function, it is used there to check if the target of the click event is a descendant of the element being rendered by the InternalOverlay component
|
fix: #1562
Changes proposed in this PR:
Implement MultiSelect component
Add tests to MultiSelect component
I have followed (at least) the PR section of the contributing guide.