Skip to content
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

Merged
merged 13 commits into from
Oct 24, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/**
* Teleport
* Copyright (C) 2024 Gravitational, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import React, { useState } from 'react';

import Box from 'design/Box';

import { FieldMultiInput } from './FieldMultiInput';

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,71 @@
/**
* Teleport
* Copyright (C) 2024 Gravitational, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import userEvent from '@testing-library/user-event';
import React, { useState } from 'react';

import { render, screen } from 'design/utils/testing';

import { FieldMultiInput, FieldMultiInputProps } from './FieldMultiInput';

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']);
});
139 changes: 139 additions & 0 deletions web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
/**
* Teleport
* Copyright (C) 2024 Gravitational, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

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 { useRef } 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 = [''];
}
Comment on lines +49 to +51
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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] = ''.)


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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Note on keys: using index as a key is an anti-pattern in general,

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).
Copy link
Contributor

Choose a reason for hiding this comment

The 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} />
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 length === 0. It's an arbitrary decision, I know; it just felt right for the time being, since it's easiest to populate the input (no need to click a button). I know, however, that it leaves an edge case that is unsupported, which is a single empty string. It wasn't really important for the job I was doing at the moment; perhaps if we use this component in more places, we can toggle this behavior with a boolean flag.

Copy link
Contributor

Choose a reason for hiding this comment

The 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}
`;
Loading