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
Merged
5 changes: 3 additions & 2 deletions api/proto/teleport/legacy/types/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -3319,8 +3319,9 @@ message DatabasePermission {

// KubernetesResource is the Kubernetes resource identifier.
message KubernetesResource {
// Kind specifies the Kubernetes Resource type.
// At the moment only "pod" is supported.
// Kind specifies the Kubernetes Resource type. See
// `KubernetesResourcesKinds` in `api/types/constants.go` for the list of
// supported values.
string Kind = 1 [(gogoproto.jsontag) = "kind,omitempty"];
// Namespace is the resource namespace.
// It supports wildcards.
Expand Down
14 changes: 13 additions & 1 deletion api/types/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -1270,7 +1270,14 @@ var RequestableResourceKinds = []string{
KindSAMLIdPServiceProvider,
}

// KubernetesResourcesKinds lists the supported Kubernetes resource kinds.
// The list below needs to be kept in sync with `kubernetesResourceKindOptions`
// in `web/packages/teleport/src/Roles/RoleEditor/standardmodel.ts`. (Keeping
// this comment separate to prevent it from being included in the official
// package docs.)

// KubernetesResourcesKinds lists the supported Kubernetes resource kinds. This
// is for the latest version of Role resources; roles whose version is set to
// v6 or prior only support [KindKubePod].
var KubernetesResourcesKinds = []string{
KindKubePod,
KindKubeSecret,
Expand Down Expand Up @@ -1318,6 +1325,11 @@ const (
KubeVerbPortForward = "portforward"
)

// The list below needs to be kept in sync with `kubernetesResourceVerbOptions`
// in `web/packages/teleport/src/Roles/RoleEditor/standardmodel.ts`. (Keeping
// this comment separate to prevent it from being included in the official
// package docs.)

// KubernetesVerbs lists the supported Kubernetes verbs.
var KubernetesVerbs = []string{
Wildcard,
Expand Down
5 changes: 3 additions & 2 deletions api/types/types.pb.go

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']);
});
121 changes: 121 additions & 0 deletions web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx
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 = [''];
}
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
Loading