fix: improve the MultiSelect component #1659
Conversation
Bug Fixes
Documentation
Contributors@HellWolf93, @LeandroTorresSicilia Commit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
src/components/MultiSelect/index.js
Outdated
| onFocus={onFocus} | ||
| onBlur={onBlur} | ||
| onKeyDown={handleKeyDown} | ||
| tabIndex={tabIndex} | ||
| tabIndex="-1" | ||
| ref={comboboxRef} | ||
| aria-labelledby={labelId} |
There was a problem hiding this comment.
It is not clear to me that it works here.
- Why is a div who handles the focus ?.
- Div does not support tapIndex attributes.
- What happens when the component is readOnly? who has the tapIndex, because the readOnly inputable are fucusable.
@TahimiLeonBravo why when readonly can't it be focusable?
https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly
src/components/MultiSelect/chips.js
Outdated
| /> | ||
| )); | ||
| return value.map(val => { | ||
| const onDeleteCallback = disabled || readOnly ? null : () => onDelete(val); |
There was a problem hiding this comment.
I think this logic works best in the handleDelete declaration in the parent component. It is only once and you don't have to be passing disable and readonly to this component.
There was a problem hiding this comment.
Each chip should call handleDelete with its own value, and the only way of doing it that occurred to me is this way, I will be thankful if you could explain to me how do you think it will be better
There was a problem hiding this comment.
@HellWolf93 I think whats @yvmunayev meant is to use this logic in parent component and then pass below a prop which could be null or a function then here you do not need to use disabled and readOnly and also repeat the logic
| const hasRightIcon = !!(icon && iconPosition === 'right'); | ||
| if (currentValues && currentValues.includes(name)) { | ||
| if (activeOptionName === name) { | ||
| return <StyledUncheckIcon />; | ||
| } |
There was a problem hiding this comment.
this will affect the piclklist too, how we can avoid this?
There was a problem hiding this comment.
I think we could pass down the multiple prop through context and then only render the uncheck icon when is multiple thus simple usages like in Picklist will be not affected
fix: #1645
Changes proposed in this PR:
Add isBare prop
Change variant prop behavior
Hide buttons when readonly
I have followed (at least) the PR section of the contributing guide.