diff --git a/src/common/InputDialog.tsx b/src/common/InputDialog.tsx index 5ec12c89e..4cb8759c0 100644 --- a/src/common/InputDialog.tsx +++ b/src/common/InputDialog.tsx @@ -17,12 +17,17 @@ import { ThemeTypings } from "@chakra-ui/styled-system"; import { ReactNode, useCallback, useRef, useState } from "react"; import { FormattedMessage } from "react-intl"; +export interface InputValidationResult { + ok: boolean; + message?: string; +} + export interface InputDialogBody { value: T; setValue: (value: T) => void; - error: string | undefined; - setError: (error: string | undefined) => void; - validate: (value: T) => string | undefined; + validationResult: InputValidationResult; + setValidationResult: (value: InputValidationResult) => void; + validate: (value: T) => InputValidationResult; } type ValueOrCancelled = T | undefined; @@ -33,13 +38,13 @@ export interface InputDialogProps { initialValue: T; actionLabel: string; size?: ThemeTypings["components"]["Modal"]["sizes"]; - validate?: (input: T) => string | undefined; + validate?: (input: T) => InputValidationResult; customFocus?: boolean; finalFocusRef?: React.RefObject; callback: (value: ValueOrCancelled) => void; } -const noValidation = () => undefined; +const noValidation = () => ({ ok: true }); /** * General purpose input dialog. @@ -56,12 +61,13 @@ export const InputDialog = ({ callback, }: InputDialogProps) => { const [value, setValue] = useState(initialValue); - const [error, setError] = useState(undefined); + const [validationResult, setValidationResult] = + useState(validate(initialValue)); const leastDestructiveRef = useRef(null); const onCancel = useCallback(() => callback(undefined), [callback]); const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); - if (!error) { + if (validationResult.ok) { callback(value); } }; @@ -87,8 +93,8 @@ export const InputDialog = ({ @@ -102,7 +108,7 @@ export const InputDialog = ({ variant="solid" onClick={handleSubmit} ml={3} - isDisabled={Boolean(error)} + isDisabled={!validationResult.ok} > {actionLabel} diff --git a/src/project/ChooseMainScriptQuestion.test.tsx b/src/project/ChooseMainScriptQuestion.test.tsx index 017f72c60..ba30e3f25 100644 --- a/src/project/ChooseMainScriptQuestion.test.tsx +++ b/src/project/ChooseMainScriptQuestion.test.tsx @@ -11,6 +11,7 @@ import ChooseMainScriptQuestion, { import { MainScriptChoice } from "./project-actions"; import { stubIntl as intl } from "../messages/testing"; import FixedTranslationProvider from "../messages/FixedTranslationProvider"; +import { InputValidationResult } from "../common/InputDialog"; describe("ChooseMainScriptQuestion", () => { const data = () => Promise.resolve(new Uint8Array([0])); @@ -19,13 +20,13 @@ describe("ChooseMainScriptQuestion", () => { const setValue = jest.fn() as jest.MockedFunction< (x: MainScriptChoice | undefined) => void >; - const setError = jest.fn() as jest.MockedFunction< - (x: string | undefined) => void + const setValidationResult = jest.fn() as jest.MockedFunction< + (x: InputValidationResult) => void >; const currentFiles = new Set(["main.py", "magic.py"]); afterEach(() => { - setError.mockClear(); + setValidationResult.mockClear(); setValue.mockClear(); }); @@ -36,13 +37,13 @@ describe("ChooseMainScriptQuestion", () => { return render( undefined} + validate={() => ({ ok: true })} /> ); diff --git a/src/project/NewFileNameQuestion.tsx b/src/project/NewFileNameQuestion.tsx index 13a79e169..7bb089c62 100644 --- a/src/project/NewFileNameQuestion.tsx +++ b/src/project/NewFileNameQuestion.tsx @@ -10,6 +10,7 @@ import { FormLabel, } from "@chakra-ui/form-control"; import { Input } from "@chakra-ui/input"; +import { Text } from "@chakra-ui/react"; import { ReactNode, useEffect, useRef } from "react"; import { FormattedMessage } from "react-intl"; import { InputDialogBody } from "../common/InputDialog"; @@ -17,9 +18,9 @@ import { InputDialogBody } from "../common/InputDialog"; interface NewFileNameQuestionProps extends InputDialogBody {} const NewFileNameQuestion = ({ - error, + validationResult, value, - setError, + setValidationResult, setValue, validate, }: NewFileNameQuestionProps) => { @@ -30,7 +31,7 @@ const NewFileNameQuestion = ({ } }, []); return ( - + @@ -41,7 +42,7 @@ const NewFileNameQuestion = ({ onChange={(e) => { const value = e.target.value; setValue(value); - setError(validate(value)); + setValidationResult(validate(value)); }} autoComplete="off" autoCorrect="off" @@ -56,7 +57,23 @@ const NewFileNameQuestion = ({ }} /> - {error} + {validationResult.message && !validationResult.ok && ( + {validationResult.message} + )} + {validationResult.message && validationResult.ok && ( + // FormErrorMessage does not display when the field is valid so we need + // an equivalent for warning feedback. + + {validationResult.message} + + )} ); }; diff --git a/src/project/ProjectNameQuestion.tsx b/src/project/ProjectNameQuestion.tsx index ad0fd0692..0a912f614 100644 --- a/src/project/ProjectNameQuestion.tsx +++ b/src/project/ProjectNameQuestion.tsx @@ -17,9 +17,9 @@ import { InputDialogBody } from "../common/InputDialog"; interface ProjectNameQuestionProps extends InputDialogBody {} const ProjectNameQuestion = ({ - error, + validationResult, value, - setError, + setValidationResult, setValue, validate, }: ProjectNameQuestionProps) => { @@ -31,7 +31,7 @@ const ProjectNameQuestion = ({ } }, []); return ( - + @@ -42,15 +42,13 @@ const ProjectNameQuestion = ({ onChange={(e) => { const value = e.target.value; setValue(value); - setError(validate(value)); + setValidationResult(validate(value)); }} > - - - + {validationResult.message} ); }; diff --git a/src/project/project-actions.tsx b/src/project/project-actions.tsx index 133840def..a0c6d39ec 100644 --- a/src/project/project-actions.tsx +++ b/src/project/project-actions.tsx @@ -755,8 +755,13 @@ export class ProjectActions { finalFocusRef={finalFocusRef} validate={(name: string) => name.trim().length === 0 - ? this.intl.formatMessage({ id: "project-name-not-empty" }) - : undefined + ? { + ok: false, + message: this.intl.formatMessage({ + id: "project-name-not-empty", + }), + } + : { ok: true } } /> )); diff --git a/src/project/project-utils.test.ts b/src/project/project-utils.test.ts index c66f1b4c5..d5dff86fd 100644 --- a/src/project/project-utils.test.ts +++ b/src/project/project-utils.test.ts @@ -12,47 +12,58 @@ describe("validateNewFilename", () => { const exists = (filename: string) => filename === "main.py"; it("requires non-empty name", () => { - expect(validateNewFilename("", exists, intl)).toEqual( - "file-name-not-empty" - ); + expect(validateNewFilename("", exists, intl)).toEqual({ + ok: false, + message: "file-name-not-empty", + }); }); it("length", () => { - expect(validateNewFilename("a".repeat(121), exists, intl)).toEqual( - "file-name-length" - ); - expect(validateNewFilename("a".repeat(120), exists, intl)).toBeUndefined(); + expect(validateNewFilename("a".repeat(121), exists, intl)).toEqual({ + ok: false, + message: "file-name-length", + }); + expect(validateNewFilename("a".repeat(120), exists, intl)).toEqual({ + ok: true, + }); }); it("no error for Python extensions", () => { - expect(validateNewFilename("foo.py", exists, intl)).toBeUndefined(); + expect(validateNewFilename("foo.py", exists, intl)).toEqual({ ok: true }); }); it("errors for spaces", () => { expect(validateNewFilename("spaces are not allowed", exists, intl)).toEqual( - "file-name-whitespace" + { + ok: false, + message: "file-name-whitespace", + } ); }); it("errors for other invalid chars", () => { - expect(validateNewFilename("wow!64", exists, intl)).toEqual( - "file-name-invalid-character" - ); + expect(validateNewFilename("wow!64", exists, intl)).toEqual({ + ok: false, + message: "file-name-invalid-character", + }); }); it("errors for leading number", () => { - expect(validateNewFilename("99greenbottles", exists, intl)).toEqual( - "file-name-start-number" - ); + expect(validateNewFilename("99greenbottles", exists, intl)).toEqual({ + ok: false, + message: "file-name-start-number", + }); }); it("errors for uppercase", () => { - expect(validateNewFilename("OHNO", exists, intl)).toEqual( - "file-name-lowercase-only" - ); + expect(validateNewFilename("OHNO", exists, intl)).toEqual({ + ok: true, + message: "file-name-lowercase-only", + }); }); it("errors for file clashes", () => { - expect(validateNewFilename("main", exists, intl)).toEqual( - "file-already-exists" - ); + expect(validateNewFilename("main", exists, intl)).toEqual({ + ok: false, + message: "file-already-exists", + }); }); it("accepts valid names", () => { expect( validateNewFilename("underscores_are_allowed", exists, intl) - ).toBeUndefined(); + ).toEqual({ ok: true }); }); }); diff --git a/src/project/project-utils.ts b/src/project/project-utils.ts index eb703fffa..7739522fe 100644 --- a/src/project/project-utils.ts +++ b/src/project/project-utils.ts @@ -6,6 +6,7 @@ import { getLowercaseFileExtension } from "../fs/fs-util"; import { IntlShape } from "react-intl"; import { isNameLengthValid } from "../fs/fs"; +import { InputValidationResult } from "../common/InputDialog"; export const isPythonFile = (filename: string) => getLowercaseFileExtension(filename) === "py"; @@ -22,7 +23,7 @@ export const isEditableFile = isPythonFile; * 1. What can be created as a file in MicroPython's file system. * 2. What can be referenced via an import in the grammar, but as it's * decidedly non-trivial, we've gone with a simplified version. - * 3. Modules should have short, all-lowercase names (PEP8) + * 3. Modules should have short, all-lowercase names (PEP8, so advice only) * * @param filename The name to check. May be user input without a file extension. * @param exists A function to check whether a file exists. @@ -31,42 +32,64 @@ export const validateNewFilename = ( filename: string, exists: (f: string) => boolean, intl: IntlShape -): string | undefined => { +): InputValidationResult => { // It's valid with or without the extension. Run checks without. if (isPythonFile(filename)) { filename = filename.slice(0, -".py".length); } if (filename.length === 0) { - return intl.formatMessage({ id: "file-name-not-empty" }); + return { + ok: false, + message: intl.formatMessage({ id: "file-name-not-empty" }), + }; } if (!isNameLengthValid(filename)) { - return intl.formatMessage({ id: "file-name-length" }); + return { + ok: false, + message: intl.formatMessage({ id: "file-name-length" }), + }; } if (filename.match(/\s/)) { - return intl.formatMessage({ id: "file-name-whitespace" }); + return { + ok: false, + message: intl.formatMessage({ id: "file-name-whitespace" }), + }; } // This includes Unicode ranges that we should exclude, but it's // probably not worth the bundle size to do this correctly. // If we revisit see Pyright's implementation. const invalidCharMatch = /[^a-zA-Z0-9_\u{a1}-\u{10ffff}]/u.exec(filename); if (invalidCharMatch) { - return intl.formatMessage( - { id: "file-name-invalid-character" }, - { - invalid: invalidCharMatch[0], - } - ); + return { + ok: false, + message: intl.formatMessage( + { id: "file-name-invalid-character" }, + { + invalid: invalidCharMatch[0], + } + ), + }; } if (filename.match("^[0-9]")) { - return intl.formatMessage({ id: "file-name-start-number" }); + return { + ok: false, + message: intl.formatMessage({ id: "file-name-start-number" }), + }; } - // This one is PEP8 so we can drop it if it causes folks practical issues. if (filename.match("[A-Z]")) { - return intl.formatMessage({ id: "file-name-lowercase-only" }); + return { + // This one is PEP8 so we still allow you to proceed. + // See https://github.com/microbit-foundation/python-editor-v3/issues/1026 + ok: true, + message: intl.formatMessage({ id: "file-name-lowercase-only" }), + }; } if (exists(ensurePythonExtension(filename))) { - return intl.formatMessage({ id: "file-already-exists" }); + return { + ok: false, + message: intl.formatMessage({ id: "file-already-exists" }), + }; } - return undefined; + return { ok: true }; };