feat(form): better defaults for validation

mlaursen · Nov 24, 2020 · 4003a07 · 4003a07
1 parent e8fb252
commit 4003a07
Show file tree

Hide file tree

Showing 6 changed files with 79 additions and 70 deletions.
diff --git a/packages/form/src/text-field/__tests__/PasswordWithMessage.tsx b/packages/form/src/text-field/__tests__/PasswordWithMessage.tsx
@@ -104,7 +104,7 @@ describe("PasswordWithMessage", () => {
     expect(field).toHaveAttribute("maxLength", "20");
   });
 
-  it("should enable the error state if the value is less than the minLength or more than the maxLength", () => {
+  it("should enable the error state if the value is greater than the maxLength", () => {
     const { getByPlaceholderText, getByRole } = render(
       <Test minLength={5} maxLength={20} messageRole="alert" />
     );
@@ -116,8 +116,8 @@ describe("PasswordWithMessage", () => {
     expect(message.className).not.toContain("--error");
 
     fireEvent.change(field, { target: { value: "1" } });
-    expect(container.className).toContain("--error");
-    expect(message.className).toContain("--error");
+    expect(container.className).not.toContain("--error");
+    expect(message.className).not.toContain("--error");
 
     fireEvent.change(field, { target: { value: "Valid" } });
     expect(container.className).not.toContain("--error");

diff --git a/packages/form/src/text-field/__tests__/TextAreaWithMessage.tsx b/packages/form/src/text-field/__tests__/TextAreaWithMessage.tsx
@@ -38,7 +38,6 @@ describe("TextAreaWithMessage", () => {
 
     expect(field).toHaveAttribute("aria-describedby", "field-id-message");
     expect(field.textContent).toBe("");
-    /* expect(field.value).toBe(""); */
 
     fireEvent.change(field, { target: { value: "a" } });
     expect(field.textContent).toBe("a");
@@ -99,7 +98,7 @@ describe("TextAreaWithMessage", () => {
     expect(field).toHaveAttribute("maxLength", "20");
   });
 
-  it("should enable the error state if the value is less than the minLength or more than the maxLength", () => {
+  it("should enable the error state if the value is greater than the maxLength", () => {
     const { getByRole } = render(
       <Test minLength={5} maxLength={20} messageRole="alert" />
     );
@@ -111,8 +110,8 @@ describe("TextAreaWithMessage", () => {
     expect(message.className).not.toContain("--error");
 
     fireEvent.change(field, { target: { value: "1" } });
-    expect(container.className).toContain("--error");
-    expect(message.className).toContain("--error");
+    expect(container.className).not.toContain("--error");
+    expect(message.className).not.toContain("--error");
 
     fireEvent.change(field, { target: { value: "Valid" } });
     expect(container.className).not.toContain("--error");
@@ -134,7 +133,7 @@ describe("TextAreaWithMessage", () => {
   it("should not update the error state on change or update the value if the custon onChange event stopped propagation", () => {
     const { getByRole } = render(
       <Test
-        minLength={10}
+        maxLength={3}
         onChange={(event) => event.stopPropagation()}
         messageRole="alert"
       />
@@ -154,7 +153,7 @@ describe("TextAreaWithMessage", () => {
 
   it("should not update the error state on change if `validateOnChange` is false", () => {
     const { getByRole } = render(
-      <Test minLength={10} validateOnChange={false} messageRole="alert" />
+      <Test maxLength={3} validateOnChange={false} messageRole="alert" />
     );
 
     const field = getByRole("textbox");
@@ -171,7 +170,7 @@ describe("TextAreaWithMessage", () => {
 
   it("should not update the error state on change if `validateOnChange` is an empty array", () => {
     const { getByRole } = render(
-      <Test minLength={10} validateOnChange={[]} messageRole="alert" />
+      <Test maxLength={3} validateOnChange={[]} messageRole="alert" />
     );
 
     const field = getByRole("textbox");
@@ -214,7 +213,7 @@ describe("TextAreaWithMessage", () => {
 
   it("should render an icon next to the text field when there is an error by default", () => {
     const { getByRole, getByText } = render(
-      <Test minLength={10} errorIcon={<ErrorOutlineFontIcon />} />
+      <Test maxLength={3} errorIcon={<ErrorOutlineFontIcon />} />
     );
     const field = getByRole("textbox");
 
@@ -226,7 +225,7 @@ describe("TextAreaWithMessage", () => {
   it("should default to the icon from the IconProvider", () => {
     const { getByText, getByRole } = render(
       <IconProvider>
-        <Test minLength={10} />
+        <Test maxLength={3} />
       </IconProvider>
     );
     const field = getByRole("textbox");
@@ -239,7 +238,7 @@ describe("TextAreaWithMessage", () => {
   it("should override the IconProvider error icon when the errorIcon prop is defined", () => {
     const { getByRole, getByText, rerender } = render(
       <IconProvider>
-        <Test minLength={10} errorIcon={null} />
+        <Test maxLength={3} errorIcon={null} />
       </IconProvider>
     );
     const field = getByRole("textbox");
@@ -250,7 +249,7 @@ describe("TextAreaWithMessage", () => {
 
     rerender(
       <IconProvider>
-        <Test minLength={10} errorIcon={<span>My Icon!</span>} />
+        <Test maxLength={3} errorIcon={<span>My Icon!</span>} />
       </IconProvider>
     );
     expect(() => getByText("My Icon!")).not.toThrow();

diff --git a/packages/form/src/text-field/__tests__/TextFieldWithMessage.tsx b/packages/form/src/text-field/__tests__/TextFieldWithMessage.tsx
@@ -100,7 +100,7 @@ describe("TextFieldWithMessage", () => {
     expect(field).toHaveAttribute("maxLength", "20");
   });
 
-  it("should enable the error state if the value is less than the minLength or more than the maxLength", () => {
+  it("should enable the error state if the value is greater than the maxLength", () => {
     const { getByRole } = render(
       <Test minLength={5} maxLength={20} messageRole="alert" />
     );
@@ -112,8 +112,8 @@ describe("TextFieldWithMessage", () => {
     expect(message.className).not.toContain("--error");
 
     fireEvent.change(field, { target: { value: "1" } });
-    expect(container.className).toContain("--error");
-    expect(message.className).toContain("--error");
+    expect(container.className).not.toContain("--error");
+    expect(message.className).not.toContain("--error");
 
     fireEvent.change(field, { target: { value: "Valid" } });
     expect(container.className).not.toContain("--error");
@@ -215,7 +215,7 @@ describe("TextFieldWithMessage", () => {
 
   it("should render an icon next to the text field when there is an error by default", () => {
     const { getByRole, getByText } = render(
-      <Test minLength={10} errorIcon={<ErrorOutlineFontIcon />} />
+      <Test maxLength={3} errorIcon={<ErrorOutlineFontIcon />} />
     );
     const field = getByRole("textbox");
 
@@ -227,7 +227,7 @@ describe("TextFieldWithMessage", () => {
   it("should default to the icon from the IconProvider", () => {
     const { getByText, getByRole } = render(
       <IconProvider>
-        <Test minLength={10} />
+        <Test maxLength={3} />
       </IconProvider>
     );
     const field = getByRole("textbox");
@@ -240,7 +240,7 @@ describe("TextFieldWithMessage", () => {
   it("should override the IconProvider error icon when the errorIcon prop is defined", () => {
     const { getByRole, getByText, rerender } = render(
       <IconProvider>
-        <Test minLength={10} errorIcon={null} />
+        <Test maxLength={3} errorIcon={null} />
       </IconProvider>
     );
     const field = getByRole("textbox");
@@ -251,7 +251,7 @@ describe("TextFieldWithMessage", () => {
 
     rerender(
       <IconProvider>
-        <Test minLength={10} errorIcon={<span>My Icon!</span>} />
+        <Test maxLength={3} errorIcon={<span>My Icon!</span>} />
       </IconProvider>
     );
     expect(() => getByText("My Icon!")).not.toThrow();
@@ -306,7 +306,7 @@ describe("TextFieldWithMessage", () => {
       <Test
         onBlur={(event) => event.stopPropagation()}
         messageRole="alert"
-        minLength={10}
+        maxLength={3}
         validateOnChange={false}
       />
     );
@@ -354,7 +354,7 @@ describe("TextFieldWithMessage", () => {
   it("should allow for a custom isErrored function", () => {
     const isErrored = jest.fn(() => false);
     const { getByRole } = render(
-      <Test isErrored={isErrored} messageRole="alert" minLength={10} />
+      <Test isErrored={isErrored} messageRole="alert" maxLength={3} />
     );
 
     expect(isErrored).not.toBeCalled();
@@ -371,7 +371,7 @@ describe("TextFieldWithMessage", () => {
     expect(isErrored).toBeCalledWith({
       value: "invalid",
       errorMessage: "",
-      minLength: 10,
+      maxLength: 3,
       isBlurEvent: false,
       validateOnChange: "recommended",
       validationMessage: "",
@@ -382,15 +382,15 @@ describe("TextFieldWithMessage", () => {
   it("should call the onErrorChange option correctly", () => {
     const onErrorChange = jest.fn();
     const { getByRole } = render(
-      <Test onErrorChange={onErrorChange} minLength={10} />
+      <Test onErrorChange={onErrorChange} maxLength={3} />
     );
 
     expect(onErrorChange).not.toBeCalled();
     const field = getByRole("textbox");
     fireEvent.change(field, { target: { value: "invalid" } });
     expect(onErrorChange).toBeCalledWith("field-id", true);
 
-    fireEvent.change(field, { target: { value: "this is a valid string" } });
+    fireEvent.change(field, { target: { value: "v" } });
     expect(onErrorChange).toBeCalledWith("field-id", false);
     expect(onErrorChange).toBeCalledTimes(2);
   });
@@ -403,7 +403,7 @@ describe("TextFieldWithMessage", () => {
     const errorIcon = <span data-testid="error-icon" />;
 
     const { getByTestId, getByRole } = render(
-      <Test minLength={10} errorIcon={errorIcon} getErrorIcon={getErrorIcon} />
+      <Test maxLength={3} errorIcon={errorIcon} getErrorIcon={getErrorIcon} />
     );
     const field = getByRole("textbox");
 

diff --git a/packages/form/src/text-field/__tests__/getErrorMessage.ts b/packages/form/src/text-field/__tests__/getErrorMessage.ts
@@ -87,7 +87,7 @@ describe("defaultGetErrorMessage", () => {
     ).toBe("");
   });
 
-  it("should only return the validity message when the validateOnChange is set to recommeded and one of the non RECOMMEDNED_IGNORED_KEYS are errored", () => {
+  it("should only return the validity message when the validateOnChange is set to recommeded and one of the RECOMMENDED_STATE_KEYS are errored", () => {
     const validate = (key: keyof ValidityState, expected: string): void => {
       expect(
         defaultGetErrorMessage({
@@ -98,16 +98,16 @@ describe("defaultGetErrorMessage", () => {
       ).toBe(expected);
     };
 
-    validate("badInput", "");
-    validate("customError", validationMessage);
-    validate("patternMismatch", validationMessage);
-    validate("rangeOverflow", validationMessage);
-    validate("rangeUnderflow", validationMessage);
-    validate("stepMismatch", validationMessage);
-    validate("tooLong", "");
+    validate("badInput", validationMessage);
+    validate("customError", "");
+    validate("patternMismatch", "");
+    validate("rangeOverflow", "");
+    validate("rangeUnderflow", "");
+    validate("stepMismatch", "");
+    validate("tooLong", validationMessage);
     validate("tooShort", "");
-    validate("typeMismatch", validationMessage);
-    validate("valueMissing", "");
+    validate("typeMismatch", "");
+    validate("valueMissing", validationMessage);
   });
 
   it("should only return the validation message for the provided validity state key", () => {

diff --git a/packages/form/src/text-field/getErrorMessage.ts b/packages/form/src/text-field/getErrorMessage.ts
@@ -67,24 +67,45 @@ export interface ErrorMessageOptions extends TextConstraints {
  */
 export type GetErrorMessage = (options: ErrorMessageOptions) => string;
 
-/** @internal */
-const RECOMMENDED_IGNORED_KEYS: readonly (keyof ValidityState)[] = [
+const VALIDITY_STATE_KEYS: readonly (keyof ValidityState)[] = [
   "badInput",
+  "customError",
+  "patternMismatch",
+  "rangeOverflow",
+  "rangeUnderflow",
+  "stepMismatch",
   "tooLong",
   "tooShort",
+  "typeMismatch",
+  "valueMissing",
+];
+
+/** @internal */
+const RECOMMENDED_STATE_KEYS: readonly (keyof ValidityState)[] = [
+  "badInput",
+  "tooLong",
   "valueMissing",
 ];
 
 /**
- * The default implementation for getting an error message for the `TextField`
- * or `TextArea` components that:
- *
- * - prevents the browser `minLength` and `tooLong` error text from appearing
- *   during change events since the message is extremely verbose
- * - prevents the `valueMissing` and `badInput` error text from appearing during
- *   change events since it's better to wait for the blur event.
+ * The validation message is actually kind of weird since it's possible for a
+ * form element to have multiple errors at once. The validation message will be
+ * the first error that appears, so need to make sure that the first error is
+ * one of the recommended state keys so the message appears for only those types
+ * of errors.
  *
- * The above behavior is also configured by the {@link ChangeValidationBehavior}.
+ * @internal
+ */
+const isRecommended = (validity: ValidityState): boolean =>
+  VALIDITY_STATE_KEYS.every((key) => {
+    const errored = validity[key];
+    return !errored || RECOMMENDED_STATE_KEYS.includes(key);
+  });
+
+/**
+ * The default implementation for getting an error message for the `TextField`
+ * or `TextArea` components that relies on the behavior of the
+ * {@link ChangeValidationBehavior}
  */
 export const defaultGetErrorMessage: GetErrorMessage = ({
   isBlurEvent,
@@ -101,28 +122,16 @@ export const defaultGetErrorMessage: GetErrorMessage = ({
   }
 
   if (validateOnChange === "recommended") {
-    return Object.entries(validity).some(
-      ([key, errored]) =>
-        errored &&
-        !RECOMMENDED_IGNORED_KEYS.includes(key as keyof ValidityState)
-    )
-      ? validationMessage
-      : "";
+    return isRecommended(validity) ? validationMessage : "";
   }
 
-  if (typeof validateOnChange === "string") {
-    return validity[validateOnChange] ? validationMessage : "";
-  }
-
-  if (
-    !validateOnChange.length ||
-    !Object.entries(validity).some(
-      ([key, errored]) =>
-        errored && validateOnChange.includes(key as keyof ValidityState)
-    )
-  ) {
-    return "";
-  }
+  const keys =
+    typeof validateOnChange === "string"
+      ? [validateOnChange]
+      : validateOnChange;
 
-  return validationMessage;
+  return keys.length &&
+    VALIDITY_STATE_KEYS.some((key) => validity[key] && keys.includes(key))
+    ? validationMessage
+    : "";
 };
diff --git a/packages/form/src/text-field/isErrored.ts b/packages/form/src/text-field/isErrored.ts
@@ -28,7 +28,8 @@ export const defaultIsErrored: IsErrored = ({
   errorMessage,
   minLength,
   maxLength,
+  isBlurEvent,
 }) =>
   !!errorMessage ||
-  (typeof minLength === "number" && value.length < minLength) ||
-  (typeof maxLength === "number" && value.length > maxLength);
+  (typeof maxLength === "number" && value.length > maxLength) ||
+  (isBlurEvent && typeof minLength === "number" && value.length < minLength);