Relax create file lowercase validation check (#1031)

Allow lowercase names as it's just a PEP8 check. Some rearrangement to keep the message in an "ok" case. Also fixes issue where the create button is enabled for a blank name until you first type (no harm came from this). See #1026
microbit-foundation · Oct 14, 2022 · 4dd60cc · 4dd60cc
1 parent 7562853
commit 4dd60cc
Show file tree

Hide file tree

Showing 7 changed files with 129 additions and 68 deletions.
diff --git 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<T> {
   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> = T | undefined;
@@ -33,13 +38,13 @@ export interface InputDialogProps<T> {
   initialValue: T;
   actionLabel: string;
   size?: ThemeTypings["components"]["Modal"]["sizes"];
-  validate?: (input: T) => string | undefined;
+  validate?: (input: T) => InputValidationResult;
   customFocus?: boolean;
   finalFocusRef?: React.RefObject<HTMLButtonElement>;
   callback: (value: ValueOrCancelled<T>) => void;
 }
 
-const noValidation = () => undefined;
+const noValidation = () => ({ ok: true });
 
 /**
  * General purpose input dialog.
@@ -56,12 +61,13 @@ export const InputDialog = <T,>({
   callback,
 }: InputDialogProps<T>) => {
   const [value, setValue] = useState(initialValue);
-  const [error, setError] = useState<string | undefined>(undefined);
+  const [validationResult, setValidationResult] =
+    useState<InputValidationResult>(validate(initialValue));
   const leastDestructiveRef = useRef<HTMLButtonElement>(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 = <T,>({
                 <Body
                   value={value}
                   setValue={setValue}
-                  error={error}
-                  setError={setError}
+                  validationResult={validationResult}
+                  setValidationResult={setValidationResult}
                   validate={validate}
                 />
               </Box>
@@ -102,7 +108,7 @@ export const InputDialog = <T,>({
               variant="solid"
               onClick={handleSubmit}
               ml={3}
-              isDisabled={Boolean(error)}
+              isDisabled={!validationResult.ok}
             >
               {actionLabel}
             </Button>

diff --git 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(
         <FixedTranslationProvider>
           <ChooseMainScriptQuestion
-            error={undefined}
-            setError={setError}
+            validationResult={{ ok: true }}
+            setValidationResult={setValidationResult}
             setValue={setValue}
             currentFiles={currentFiles}
             value={{ main: choice }}
             inputs={inputs}
-            validate={() => undefined}
+            validate={() => ({ ok: true })}
           />
         </FixedTranslationProvider>
       );

diff --git a/src/project/NewFileNameQuestion.tsx b/src/project/NewFileNameQuestion.tsx
@@ -10,16 +10,17 @@ 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";
 
 interface NewFileNameQuestionProps extends InputDialogBody<string> {}
 
 const NewFileNameQuestion = ({
-  error,
+  validationResult,
   value,
-  setError,
+  setValidationResult,
   setValue,
   validate,
 }: NewFileNameQuestionProps) => {
@@ -30,7 +31,7 @@ const NewFileNameQuestion = ({
     }
   }, []);
   return (
-    <FormControl id="fileName" isRequired isInvalid={Boolean(error)}>
+    <FormControl id="fileName" isRequired isInvalid={!validationResult.ok}>
       <FormLabel>
         <FormattedMessage id="name-text" />
       </FormLabel>
@@ -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 = ({
           }}
         />
       </FormHelperText>
-      <FormErrorMessage>{error}</FormErrorMessage>
+      {validationResult.message && !validationResult.ok && (
+        <FormErrorMessage>{validationResult.message}</FormErrorMessage>
+      )}
+      {validationResult.message && validationResult.ok && (
+        // FormErrorMessage does not display when the field is valid so we need
+        // an equivalent for warning feedback.
+        <Text
+          id="fileName-feedback"
+          aria-live="polite"
+          fontSize="sm"
+          color="red.500"
+          lineHeight="normal"
+          mt={2}
+        >
+          {validationResult.message}
+        </Text>
+      )}
     </FormControl>
   );
 };

diff --git a/src/project/ProjectNameQuestion.tsx b/src/project/ProjectNameQuestion.tsx
@@ -17,9 +17,9 @@ import { InputDialogBody } from "../common/InputDialog";
 interface ProjectNameQuestionProps extends InputDialogBody<string> {}
 
 const ProjectNameQuestion = ({
-  error,
+  validationResult,
   value,
-  setError,
+  setValidationResult,
   setValue,
   validate,
 }: ProjectNameQuestionProps) => {
@@ -31,7 +31,7 @@ const ProjectNameQuestion = ({
     }
   }, []);
   return (
-    <FormControl id="fileName" isRequired isInvalid={Boolean(error)}>
+    <FormControl id="fileName" isRequired isInvalid={!validationResult.ok}>
       <FormLabel>
         <FormattedMessage id="name-text" />
       </FormLabel>
@@ -42,15 +42,13 @@ const ProjectNameQuestion = ({
         onChange={(e) => {
           const value = e.target.value;
           setValue(value);
-          setError(validate(value));
+          setValidationResult(validate(value));
         }}
       ></Input>
       <FormHelperText color="gray.700">
         <FormattedMessage id="name-used-when" />
       </FormHelperText>
-      <FormErrorMessage>
-        <FormattedMessage id={error} />
-      </FormErrorMessage>
+      <FormErrorMessage>{validationResult.message}</FormErrorMessage>
     </FormControl>
   );
 };

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