-
-
Notifications
You must be signed in to change notification settings - Fork 112
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
fix: improve the MultiSelect component #1659
Changes from 1 commit
e3894f1
0db1cd5
d5e58e1
7f8a4b0
04907dd
c5bfda9
6c1ba86
6285055
e3c7acc
f19835d
95fd79f
a9cc0bf
b113f69
a7b348b
f64985a
4600f8a
8607e07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
const PageMultiSelect = require('../../../src/components/MultiSelect/pageObject'); | ||
|
||
const MULTI_SELECT = '#multiselect-component-9'; | ||
|
||
describe('MultiSelect base', () => { | ||
beforeAll(() => { | ||
browser.url('/#!/MultiSelect/9'); | ||
}); | ||
beforeEach(() => { | ||
browser.refresh(); | ||
const component = $(MULTI_SELECT); | ||
component.waitForExist(); | ||
}); | ||
|
||
it('should not put the input element focused when clicked', () => { | ||
const input = new PageMultiSelect(MULTI_SELECT); | ||
input.click(); | ||
expect(input.hasFocus()).toBe(false); | ||
}); | ||
|
||
it('should not put the input element focused when the label element is clicked', () => { | ||
const input = new PageMultiSelect(MULTI_SELECT); | ||
input.clickLabel(); | ||
expect(input.hasFocus()).toBe(false); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import getContent from '../getContent'; | ||
|
||
describe('getContent', () => { | ||
it('should return null', () => { | ||
const values = [false, true, undefined, null]; | ||
values.forEach(value => { | ||
expect(getContent(value)).toBe(null); | ||
}); | ||
}); | ||
|
||
it('should return the right string', () => { | ||
const values = [{ label: 'Label' }, [{ label: 'Label 1' }, { label: 'Label 2' }]]; | ||
const expected = ['Label', 'Label 1, Label 2']; | ||
values.forEach((value, index) => { | ||
expect(getContent(value)).toBe(expected[index]); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
export default function getContent(value) { | ||
if (!value || typeof value !== 'object') { | ||
return null; | ||
} | ||
if (Array.isArray(value)) { | ||
return value.map(item => item.label).join(', '); | ||
} | ||
return value.label; | ||
LeandroTorresSicilia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import { | |
useErrorMessageId, | ||
useReduxForm, | ||
useLabelId, | ||
useWindowResize, | ||
} from '../../libs/hooks'; | ||
import { | ||
StyledInput, | ||
|
@@ -15,6 +16,7 @@ import { | |
StyledButtonIcon, | ||
StyledPlaceholder, | ||
StyledCombobox, | ||
StyledText, | ||
} from './styled'; | ||
import InternalDropdown from '../InternalDropdown'; | ||
import InternalOverlay from '../InternalOverlay'; | ||
|
@@ -26,6 +28,7 @@ import { ENTER_KEY, SPACE_KEY, ESCAPE_KEY, TAB_KEY } from '../../libs/constants' | |
import { hasChips, positionResolver } from './helpers'; | ||
import Chips from './chips'; | ||
import normalizeValue from './helpers/normalizeValue'; | ||
import getContent from './helpers/getContent'; | ||
|
||
const MultiSelect = React.forwardRef((props, ref) => { | ||
const { | ||
|
@@ -40,9 +43,10 @@ const MultiSelect = React.forwardRef((props, ref) => { | |
required, | ||
disabled, | ||
readOnly, | ||
tabIndex, | ||
tabIndex: tabIndexInProps, | ||
variant, | ||
chipVariant, | ||
isBare, | ||
value, | ||
onChange, | ||
onFocus, | ||
|
@@ -55,13 +59,13 @@ const MultiSelect = React.forwardRef((props, ref) => { | |
const comboboxRef = useRef(); | ||
useImperativeHandle(ref, () => ({ | ||
focus: () => { | ||
comboboxRef.current.focus(); | ||
triggerRef.current.focus(); | ||
}, | ||
click: () => { | ||
comboboxRef.current.click(); | ||
triggerRef.current.click(); | ||
}, | ||
blur: () => { | ||
comboboxRef.current.blur(); | ||
triggerRef.current.blur(); | ||
}, | ||
})); | ||
|
||
|
@@ -77,7 +81,7 @@ const MultiSelect = React.forwardRef((props, ref) => { | |
setIsOpen(false); | ||
// eslint-disable-next-line no-use-before-define | ||
stopListeningOutsideClick(); | ||
comboboxRef.current.focus(); | ||
setTimeout(() => triggerRef.current.focus(), 0); | ||
}; | ||
|
||
const handleChange = val => { | ||
|
@@ -141,8 +145,22 @@ const MultiSelect = React.forwardRef((props, ref) => { | |
dropdownRef, | ||
handleOutsideClick, | ||
); | ||
useWindowResize(() => setIsOpen(false), isOpen); | ||
|
||
const shouldRenderChips = hasChips(value); | ||
const tabIndex = disabled || readOnly ? '-1' : tabIndexInProps; | ||
LeandroTorresSicilia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const content = | ||
variant === 'chip' ? ( | ||
<Chips | ||
value={value} | ||
variant={chipVariant} | ||
readOnly={readOnly} | ||
disabled={disabled} | ||
onDelete={handleDelete} | ||
/> | ||
) : ( | ||
<StyledText>{getContent(value)}</StyledText> | ||
); | ||
LeandroTorresSicilia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return ( | ||
<StyledContainer id={id} className={className} style={style}> | ||
|
@@ -155,7 +173,7 @@ const MultiSelect = React.forwardRef((props, ref) => { | |
/> | ||
<StyledCombobox | ||
id={comboboxId} | ||
variant={variant} | ||
isBare={isBare} | ||
error={error} | ||
disabled={disabled} | ||
role="combobox" | ||
|
@@ -164,7 +182,7 @@ const MultiSelect = React.forwardRef((props, ref) => { | |
onFocus={onFocus} | ||
onBlur={onBlur} | ||
onKeyDown={handleKeyDown} | ||
tabIndex={tabIndex} | ||
tabIndex="-1" | ||
ref={comboboxRef} | ||
aria-labelledby={labelId} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not clear to me that it works here.
@TahimiLeonBravo why when readonly can't it be focusable? https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
> | ||
|
@@ -181,20 +199,20 @@ const MultiSelect = React.forwardRef((props, ref) => { | |
<RenderIf isTrue={!shouldRenderChips}> | ||
<StyledPlaceholder>{placeholder}</StyledPlaceholder> | ||
</RenderIf> | ||
<RenderIf isTrue={shouldRenderChips}> | ||
<Chips value={value} variant={chipVariant} onDelete={handleDelete} /> | ||
</RenderIf> | ||
<RenderIf isTrue={shouldRenderChips}>{content}</RenderIf> | ||
LeandroTorresSicilia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</StyledChipContainer> | ||
<StyledButtonIcon | ||
title="Add" | ||
variant="neutral" | ||
size="small" | ||
icon={<PlusIcon />} | ||
onClick={handleTriggerClick} | ||
disabled={disabled} | ||
ref={triggerRef} | ||
tabIndex="-1" | ||
/> | ||
<RenderIf isTrue={!readOnly && !disabled}> | ||
<StyledButtonIcon | ||
title="Add" | ||
variant="neutral" | ||
size="small" | ||
icon={<PlusIcon />} | ||
onClick={handleTriggerClick} | ||
disabled={disabled} | ||
ref={triggerRef} | ||
tabIndex={tabIndex} | ||
/> | ||
</RenderIf> | ||
<InternalOverlay | ||
isVisible={isOpen} | ||
positionResolver={positionResolver} | ||
|
@@ -250,9 +268,11 @@ MultiSelect.propTypes = { | |
/** Specifies the tab order of an element (when the tab button is used for navigating). */ | ||
tabIndex: PropTypes.oneOfType([PropTypes.number, PropTypes.string]), | ||
/** Specifies the variant for the input. */ | ||
variant: PropTypes.oneOf(['default', 'bare']), | ||
variant: PropTypes.oneOf(['default', 'chip']), | ||
/** Specifies the variant for the chips. */ | ||
chipVariant: PropTypes.oneOf(['base', 'neutral', 'outline-brand', 'brand']), | ||
/** Specifies that an input will not have border. This value defaults to false. */ | ||
isBare: PropTypes.bool, | ||
/** Specifies the value of an input element. */ | ||
value: PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
|
@@ -288,6 +308,7 @@ MultiSelect.defaultProps = { | |
tabIndex: 0, | ||
variant: 'default', | ||
chipVariant: 'base', | ||
isBare: false, | ||
value: undefined, | ||
onChange: () => {}, | ||
onFocus: () => {}, | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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