Merge pull request #1557 from klembot/improve-add-story-format-button

Validate story format on submit instead of input

src/components/control/__tests__/prompt-button.test.tsx

@@ -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();

src/components/control/prompt-button.css

@@ -0,0 +1,3 @@
+.prompt-button .validation-message {
+	padding: 0;
+}

src/components/control/prompt-button.tsx

@@ -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;
@@ -27,6 +28,7 @@ export interface PromptButtonProps
 	submitLabel?: string;
 	submitVariant?: IconButtonProps['variant'];
 	validate?: PromptButtonValidator;
+	validateOn?: 'change' | 'submit';
 	value: string;
 }
 
@@ -41,6 +43,7 @@ export const PromptButton: React.FC<PromptButtonProps> = props => {
 		submitLabel,
 		submitVariant,
 		validate,
+		validateOn = 'change',
 		value,
 		...other
 	} = props;
@@ -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) {
@@ -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.
 
@@ -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

src/dialogs/story-formats/__tests__/add-story-format-button.test.tsx

@@ -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(
@@ -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(
@@ -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')

src/dialogs/story-formats/add-story-format-button.tsx

@@ -97,6 +97,7 @@ export const AddStoryFormatButton: React.FC = () => {
 				submitLabel={t('common.add')}
 				submitVariant="create"
 				validate={validate}
+				validateOn="submit"
 				value={newFormatUrl}
 			/>
 		</span>

src/dialogs/story-formats/story-formats.css

@@ -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);
 }