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

Validate story format on submit instead of input #1557

Merged
merged 1 commit into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
51 changes: 35 additions & 16 deletions src/components/control/__tests__/prompt-button.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -124,23 +124,42 @@ describe('<PromptButton>', () => {
expect(onChange).toHaveBeenCalledTimes(1);
});

it('prevents submission if the validate prop blocks it', async () => {
const onSubmit = jest.fn();
it.each(['change', 'submit'])(
'prevents submission if the validate prop blocks it when the validateOn prop is "%s"',
async validateOn => {
const onSubmit = jest.fn();

renderComponent({
onSubmit,
validate,
prompt: 'test-prompt',
submitLabel: 'test-submit',
value: 'bad'
});
fireEvent.click(screen.getByRole('button'));
await waitFor(() =>
expect(screen.getByRole('button', {name: 'test-submit'})).toBeDisabled()
);
fireEvent.submit(screen.getByRole('textbox', {name: 'test-prompt'}));
expect(onSubmit).not.toHaveBeenCalled();
});
renderComponent({
onSubmit,
validate,
validateOn: validateOn as 'change' | 'submit',
prompt: 'test-prompt',
submitLabel: 'test-submit',
value: 'bad'
});
fireEvent.click(screen.getByRole('button'));

if (validateOn === 'change') {
// The button won't disable itself right away if we're validating on submit.

await waitFor(() =>
expect(
screen.getByRole('button', {name: 'test-submit'})
).toBeDisabled()
);
}
fireEvent.submit(screen.getByRole('textbox', {name: 'test-prompt'}));

if (validateOn === 'submit') {
// ... but should now be disabled if we're validating on submit.

expect(
screen.getByRole('button', {name: 'test-submit'})
).toBeDisabled();
}
expect(onSubmit).not.toHaveBeenCalled();
}
);

it('is accessible', async () => {
const {container} = renderComponent();
Expand Down
3 changes: 3 additions & 0 deletions src/components/control/prompt-button.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.prompt-button .validation-message {
padding: 0;
}
29 changes: 26 additions & 3 deletions src/components/control/prompt-button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {CardContent} from '../container/card';
import {CardButton, CardButtonProps} from './card-button';
import {IconButton, IconButtonProps} from './icon-button';
import {TextInput} from './text-input';
import './prompt-button.css';

export interface PromptValidationResponse {
message?: string;
Expand All @@ -27,6 +28,7 @@ export interface PromptButtonProps
submitLabel?: string;
submitVariant?: IconButtonProps['variant'];
validate?: PromptButtonValidator;
validateOn?: 'change' | 'submit';
value: string;
}

Expand All @@ -41,6 +43,7 @@ export const PromptButton: React.FC<PromptButtonProps> = props => {
submitLabel,
submitVariant,
validate,
validateOn = 'change',
value,
...other
} = props;
Expand All @@ -52,7 +55,7 @@ export const PromptButton: React.FC<PromptButtonProps> = props => {

React.useEffect(() => {
async function updateValidation() {
if (validate) {
if (validateOn === 'change' && validate) {
const validation = await validate(value);

if (mounted.current) {
Expand All @@ -79,9 +82,27 @@ export const PromptButton: React.FC<PromptButtonProps> = props => {
setOpen(false);
}

function handleSubmit(event: React.FormEvent) {
async function handleSubmit(event: React.FormEvent) {
event.preventDefault();

if (validateOn === 'submit' && validate) {
// Temporarily set us invalid so that the submit button is disabled while
// validation occurs, then run validation and update accordingly. If we
// fail validation here, then stop.

setValidation(value => ({...value, valid: false}));

const validation = await validate(value);

setValidation(validation);

if (!validation.valid) {
return;
}
} else {
setValidation({valid: true});
}

// It's possible to submit with the Enter key and bypass us disabling the
// submit button, so we need to catch that here.

Expand All @@ -104,7 +125,9 @@ export const PromptButton: React.FC<PromptButtonProps> = props => {
<TextInput onChange={onChange} orientation="vertical" value={value}>
{prompt}
</TextInput>
{validation?.message && <p>{validation.message}</p>}
{validation?.message && (
<p className="validation-message">{validation.message}</p>
)}
</CardContent>
<ButtonBar>
<IconButton
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,22 +75,23 @@ describe('<AddStoryFormatButton>', () => {
expect(formatInspector.dataset.version).toBe(format.version);
});

it('shows an error if an invalid URL is entered', async () => {
it('shows an error on submit if an invalid URL is entered', async () => {
await renderComponent();
fireEvent.change(
screen.getByRole('textbox', {
name: 'dialogs.storyFormats.addStoryFormatButton.prompt'
}),
{target: {value: 'not a url'}}
);
fireEvent.click(getAddButton());
await act(async () => Promise.resolve());
expect(
screen.getByText('dialogs.storyFormats.addStoryFormatButton.invalidUrl')
).toBeInTheDocument();
expect(getAddButton()).toBeDisabled();
});

it('shows an error if fetching story properties fails', async () => {
it('shows an error on submit if fetching story properties fails', async () => {
fetchStoryFormatPropertiesMock.mockRejectedValue(new Error());
await renderComponent();
fireEvent.change(
Expand All @@ -99,14 +100,15 @@ describe('<AddStoryFormatButton>', () => {
}),
{target: {value: 'http://mock-format-url'}}
);
fireEvent.click(getAddButton());
await act(async () => Promise.resolve());
expect(
screen.getByText('dialogs.storyFormats.addStoryFormatButton.fetchError')
).toBeInTheDocument();
expect(getAddButton()).toBeDisabled();
});

it('shows an error if the URL points to a format with the same name and version as a format that already exists', async () => {
it('shows an error on submit if the URL points to a format with the same name and version as a format that already exists', async () => {
const format = fakeLoadedStoryFormat();

fetchStoryFormatPropertiesMock.mockResolvedValue(
Expand All @@ -119,6 +121,7 @@ describe('<AddStoryFormatButton>', () => {
}),
{target: {value: 'http://mock-format-url'}}
);
fireEvent.click(getAddButton());
await act(async () => Promise.resolve());
expect(
screen.getByText('dialogs.storyFormats.addStoryFormatButton.alreadyAdded')
Expand Down
1 change: 1 addition & 0 deletions src/dialogs/story-formats/add-story-format-button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ export const AddStoryFormatButton: React.FC = () => {
submitLabel={t('common.add')}
submitVariant="create"
validate={validate}
validateOn="submit"
value={newFormatUrl}
/>
</span>
Expand Down
2 changes: 1 addition & 1 deletion src/dialogs/story-formats/story-formats.css
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
flex: 1 1 0;
}

.story-formats-dialog .card-content > p {
.story-formats-dialog .card-content > p:not(.validation-message) {
padding: var(--grid-size);
}

Expand Down
Loading