-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add a multi-value input component #47804
Changes from 2 commits
ebf686c
eabbf32
351faa7
6d2eda5
75c47ed
b4e8317
16fbc92
36f989c
ba129e7
960a336
f0ae6c6
af9332e
aa88cbc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import React, { useState } from 'react'; | ||
import { FieldMultiInput } from './FieldMultiInput'; | ||
import Box from 'design/Box'; | ||
|
||
export default { | ||
title: 'Shared', | ||
}; | ||
|
||
export function Story() { | ||
const [items, setItems] = useState([]); | ||
return ( | ||
<Box width="500px"> | ||
<FieldMultiInput label="Some items" value={items} onChange={setItems} /> | ||
</Box> | ||
); | ||
} | ||
Story.storyName = 'FieldMultiInput'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import userEvent from '@testing-library/user-event'; | ||
import React, { useState } from 'react'; | ||
import { FieldMultiInput, FieldMultiInputProps } from './FieldMultiInput'; | ||
import { render, screen } from 'design/utils/testing'; | ||
|
||
const TestFieldMultiInput = ({ | ||
onChange, | ||
...rest | ||
}: Partial<FieldMultiInputProps>) => { | ||
const [items, setItems] = useState<string[]>([]); | ||
const handleChange = (it: string[]) => { | ||
setItems(it); | ||
onChange?.(it); | ||
}; | ||
return <FieldMultiInput value={items} onChange={handleChange} {...rest} />; | ||
}; | ||
|
||
test('adding, editing, and removing items', async () => { | ||
const user = userEvent.setup(); | ||
const onChange = jest.fn(); | ||
render(<TestFieldMultiInput onChange={onChange} />); | ||
|
||
await user.type(screen.getByRole('textbox'), 'apples'); | ||
expect(onChange).toHaveBeenLastCalledWith(['apples']); | ||
|
||
await user.click(screen.getByRole('button', { name: 'Add More' })); | ||
expect(onChange).toHaveBeenLastCalledWith(['apples', '']); | ||
|
||
await user.type(screen.getAllByRole('textbox')[1], 'oranges'); | ||
expect(onChange).toHaveBeenLastCalledWith(['apples', 'oranges']); | ||
|
||
await user.click(screen.getAllByRole('button', { name: 'Remove Item' })[0]); | ||
expect(onChange).toHaveBeenLastCalledWith(['oranges']); | ||
|
||
await user.click(screen.getAllByRole('button', { name: 'Remove Item' })[0]); | ||
expect(onChange).toHaveBeenLastCalledWith([]); | ||
}); | ||
|
||
test('keyboard handling', async () => { | ||
const user = userEvent.setup(); | ||
const onChange = jest.fn(); | ||
render(<TestFieldMultiInput onChange={onChange} />); | ||
|
||
await user.click(screen.getByRole('textbox')); | ||
await user.keyboard('apples{Enter}oranges'); | ||
expect(onChange).toHaveBeenLastCalledWith(['apples', 'oranges']); | ||
|
||
await user.click(screen.getAllByRole('textbox')[0]); | ||
await user.keyboard('{Enter}bananas'); | ||
expect(onChange).toHaveBeenLastCalledWith(['apples', 'bananas', 'oranges']); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
import Box from 'design/Box'; | ||
import { ButtonSecondary } from 'design/Button'; | ||
import ButtonIcon from 'design/ButtonIcon'; | ||
import Flex from 'design/Flex'; | ||
import * as Icon from 'design/Icon'; | ||
import Input from 'design/Input'; | ||
import { useEffect, useRef, useState } from 'react'; | ||
import styled, { useTheme } from 'styled-components'; | ||
|
||
export type FieldMultiInputProps = { | ||
label?: string; | ||
value: string[]; | ||
disabled?: boolean; | ||
onChange?(val: string[]): void; | ||
}; | ||
|
||
/** | ||
* Allows editing a list of strings, one value per row. Use instead of | ||
* `FieldSelectCreatable` when: | ||
* | ||
* - There are no predefined values to be picked from. | ||
* - Values are expected to be relatively long and would be unreadable after | ||
* being truncated. | ||
*/ | ||
export function FieldMultiInput({ | ||
label, | ||
value, | ||
disabled, | ||
onChange, | ||
}: FieldMultiInputProps) { | ||
if (value.length === 0) { | ||
value = ['']; | ||
} | ||
|
||
const theme = useTheme(); | ||
// Index of the input to be focused after the next rendering. | ||
const toFocus = useRef<number | undefined>(); | ||
|
||
const setFocus = element => { | ||
element?.focus(); | ||
toFocus.current = undefined; | ||
}; | ||
|
||
function insertItem(index: number) { | ||
onChange?.(value.toSpliced(index, 0, '')); | ||
} | ||
|
||
function removeItem(index: number) { | ||
onChange?.(value.toSpliced(index, 1)); | ||
} | ||
|
||
function handleKeyDown(index: number, e: React.KeyboardEvent) { | ||
if (e.key === 'Enter') { | ||
insertItem(index + 1); | ||
toFocus.current = index + 1; | ||
} | ||
} | ||
|
||
return ( | ||
<Box> | ||
<Fieldset> | ||
{label && <Legend>{label}</Legend>} | ||
{value.map((val, i) => ( | ||
// Note on keys: using index as a key is an anti-pattern in general, | ||
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.
TIL 👍 |
||
// but here, we can safely assume that even though the list is | ||
// editable, we don't rely on any unmanaged HTML element state other | ||
// than focus, which we deal with separately anyway. The alternatives | ||
// would be either to require an array with keys generated | ||
// synthetically and injected from outside (which would make the API | ||
// difficult to use) or to keep the array with generated IDs as local | ||
// state (which would require us to write a prop/state reconciliation | ||
// procedure whose complexity would probably outweigh the benefits). | ||
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. sure, im convinced. |
||
<Flex key={i} alignItems="center" gap={2}> | ||
<Box flex="1"> | ||
<Input | ||
value={val} | ||
ref={toFocus.current === i ? setFocus : null} | ||
onChange={e => | ||
onChange?.( | ||
value.map((v, j) => (j === i ? e.target.value : v)) | ||
) | ||
} | ||
onKeyDown={e => handleKeyDown(i, e)} | ||
/> | ||
</Box> | ||
<ButtonIcon | ||
size="0" | ||
title="Remove Item" | ||
onClick={() => removeItem(i)} | ||
disabled={disabled} | ||
> | ||
<Icon.Cross size="small" color={theme.colors.text.muted} /> | ||
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. Love the cross here instead of trashcan, thank you! |
||
</ButtonIcon> | ||
</Flex> | ||
))} | ||
<ButtonSecondary | ||
alignSelf="start" | ||
onClick={() => insertItem(value.length)} | ||
> | ||
<Icon.Plus size="small" mr={2} /> | ||
Add More | ||
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. Related to my comment about the empty value, perhaps we don't have any input and if length === 0, this says "add field" and then "add more" if greater than 1? Idk, spitballin here. 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. The way I wrote it, I assumed that there's always at least 1 input element, and if it's empty, it simply means an empty array. Therefore there's no case where 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. Fair enough |
||
</ButtonSecondary> | ||
</Fieldset> | ||
</Box> | ||
); | ||
} | ||
|
||
const Fieldset = styled.fieldset` | ||
border: none; | ||
margin: 0; | ||
padding: 0; | ||
display: flex; | ||
flex-direction: column; | ||
gap: ${props => props.theme.space[2]}px; | ||
`; | ||
|
||
const Legend = styled.legend` | ||
margin: 0 0 ${props => props.theme.space[1]}px 0; | ||
padding: 0; | ||
${props => props.theme.typography.body3} | ||
`; |
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.
This doesn't seem right. I don't like updating the prop value here directly, especially with an array of length 1. Because arrays are passed by reference, and this is probably provided by some state, this is technically mutating state directly which we shouldn't do in react.
Can we think of another way to do 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 doesn't work this way: if you pass an array by to a function (or set it to a variable), then reassigning to it only changes the local reference, and leaves the original object alone. However, I get it that it may look ugly, so I can just introduce a new variable.
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.
(What would be a violation of rules would be saying
value[0] = ''
.)