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

Adds a model-level validation capability to our validation library #49543

Merged
merged 4 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ import React, { useState } from 'react';

import Box from 'design/Box';

import { Button } from 'design/Button';

import Validation from 'shared/components/Validation';

import { arrayOf, requiredField } from '../Validation/rules';

import { FieldMultiInput } from './FieldMultiInput';

export default {
Expand All @@ -30,7 +36,21 @@ export function Story() {
const [items, setItems] = useState([]);
return (
<Box width="500px">
<FieldMultiInput label="Some items" value={items} onChange={setItems} />
<Validation>
{({ validator }) => (
<>
<FieldMultiInput
label="Some items"
value={items}
onChange={setItems}
rule={arrayOf(requiredField('required'))}
/>
<Button mt={3} onClick={() => validator.validate()}>
Validate
</Button>
</>
)}
</Validation>
</Box>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,36 @@
import userEvent from '@testing-library/user-event';
import React, { useState } from 'react';

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

import Validation, { Validator } from 'shared/components/Validation';

import { arrayOf, requiredField } from '../Validation/rules';

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

const TestFieldMultiInput = ({
onChange,
refValidator,
...rest
}: Partial<FieldMultiInputProps>) => {
}: Partial<FieldMultiInputProps> & {
refValidator?: (v: Validator) => void;
}) => {
const [items, setItems] = useState<string[]>([]);
const handleChange = (it: string[]) => {
setItems(it);
onChange?.(it);
};
return <FieldMultiInput value={items} onChange={handleChange} {...rest} />;
return (
<Validation>
{({ validator }) => {
refValidator?.(validator);
return (
<FieldMultiInput value={items} onChange={handleChange} {...rest} />
);
}}
</Validation>
);
};

test('adding, editing, and removing items', async () => {
Expand Down Expand Up @@ -69,3 +85,35 @@ test('keyboard handling', async () => {
await user.keyboard('{Enter}bananas');
expect(onChange).toHaveBeenLastCalledWith(['apples', 'bananas', 'oranges']);
});

test('validation', async () => {
const user = userEvent.setup();
let validator: Validator;
render(
<TestFieldMultiInput
refValidator={v => {
validator = v;
}}
rule={arrayOf(requiredField('required'))}
/>
);

act(() => validator.validate());
expect(validator.state.valid).toBe(true);
expect(screen.getByRole('textbox')).toHaveAccessibleDescription('');

await user.click(screen.getByRole('button', { name: 'Add More' }));
await user.type(screen.getAllByRole('textbox')[1], 'foo');
act(() => validator.validate());
expect(validator.state.valid).toBe(false);
expect(screen.getAllByRole('textbox')[0]).toHaveAccessibleDescription(
'required'
);
expect(screen.getAllByRole('textbox')[1]).toHaveAccessibleDescription('');

await user.type(screen.getAllByRole('textbox')[0], 'foo');
act(() => validator.validate());
expect(validator.state.valid).toBe(true);
expect(screen.getAllByRole('textbox')[0]).toHaveAccessibleDescription('');
expect(screen.getAllByRole('textbox')[1]).toHaveAccessibleDescription('');
});
36 changes: 34 additions & 2 deletions web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,35 @@ 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';

import {
precomputed,
Rule,
ValidationResult,
} from 'shared/components/Validation/rules';
import { useRule } from 'shared/components/Validation';

import FieldInput from '../FieldInput';

type StringListValidationResult = ValidationResult & {
/**
* A list of validation results, one per label. Note: items are optional just
* because `useRule` by default returns only `ValidationResult`. For the
* actual validation, it's not optional; if it's undefined, or there are
* fewer items in this list than the labels, the corresponding items will be
* treated as valid.
*/
items?: ValidationResult[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if i received this field in some result i would have no idea what it meant. Based on the comment, should this be validItems or ... unchecked items? (based on label number? im confused)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avatus How about itemResults?

Copy link
Contributor

Choose a reason for hiding this comment

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

what is the context of the result? valid or not valid? checked vs unchecked? thats the part I'm getting lost with (forgive my brain)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see what you mean. The results are "valid or not valid". In other words, these are per-item validation results that enable us to show a validity state and message for each item of the string list separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhhh, that makes sense. with that context, items probably makes most sense. I think maybe even results might be better but, I'll leave it up to you if you want to make any changes

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for results.

};

export type FieldMultiInputProps = {
label?: string;
value: string[];
disabled?: boolean;
onChange?(val: string[]): void;
rule?: Rule<string[], StringListValidationResult>;
};

/**
Expand All @@ -45,7 +65,13 @@ export function FieldMultiInput({
value,
disabled,
onChange,
rule = defaultRule,
}: FieldMultiInputProps) {
// It's important to first validate, and then treat an empty array as a
// single-item list with an empty string, since this "synthetic" empty
// string is technically not a part of the model and should not be
// validated.
const validationResult: StringListValidationResult = useRule(rule(value));
if (value.length === 0) {
value = [''];
}
Expand Down Expand Up @@ -90,15 +116,19 @@ export function FieldMultiInput({
// procedure whose complexity would probably outweigh the benefits).
<Flex key={i} alignItems="center" gap={2}>
<Box flex="1">
<Input
<FieldInput
value={val}
rule={precomputed(
validationResult.items?.[i] ?? { valid: true }
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you help me understand how this is interacting with default rule? The default rule just returns valid: true, why would we need to do extra validation here based on item length? could we just return whatever the default rule returns here instead of valid: true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This { valid: true } object is a per-item object, as opposed to the one returned from the default rule, which applies to all items.

ref={toFocus.current === i ? setFocus : null}
onChange={e =>
onChange?.(
value.map((v, j) => (j === i ? e.target.value : v))
)
}
onKeyDown={e => handleKeyDown(i, e)}
mb={0}
/>
</Box>
<ButtonIcon
Expand All @@ -123,6 +153,8 @@ export function FieldMultiInput({
);
}

const defaultRule = () => () => ({ valid: true });

const Fieldset = styled.fieldset`
border: none;
margin: 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,10 @@ export function Default() {
return (
<Validation>
{({ validator }) => {
validator.validate();
// Prevent rendering loop.
if (!validator.state.validating) {
validator.validate();
}
return (
<Flex flexDirection="column">
<FieldSelect
Expand Down
105 changes: 0 additions & 105 deletions web/packages/shared/components/Validation/Validation.jsx
gzdunek marked this conversation as resolved.
Show resolved Hide resolved

This file was deleted.

Loading
Loading