Merge branch 'master' into 64608-numeric-input

frontend/src/metabase/visualizations/components/settings/ChartSettingInputNumeric.tsx

@@ -1,35 +1,19 @@
-import debounce from "lodash.debounce";
-import { type ChangeEvent, useEffect, useMemo, useState } from "react";
-import { useLatest } from "react-use";
+import { useState } from "react";
 
 import { TextInput } from "metabase/ui";
 
 import type { ChartSettingWidgetProps } from "./types";
 
-const ALLOWED_CHARS = new Set([
-  "0",
-  "1",
-  "2",
-  "3",
-  "4",
-  "5",
-  "6",
-  "7",
-  "8",
-  "9",
-  ".",
-  "-",
-  "e",
-]);
+type NumberOptions = {
+  isInteger?: boolean;
+  isNonNegative?: boolean;
+};
 
 // Note: there are more props than these that are provided by the viz settings
 // code, we just don't have types for them here.
 interface ChartSettingInputProps
   extends Omit<ChartSettingWidgetProps<number>, "onChangeSettings"> {
-  options?: {
-    isInteger?: boolean;
-    isNonNegative?: boolean;
-  };
+  options?: NumberOptions;
   id?: string;
   placeholder?: string;
   getDefault?: () => string;
@@ -40,7 +24,7 @@ export const ChartSettingInputNumeric = ({
   onChange,
   value,
   placeholder,
-  options,
+  options = {},
   id,
   getDefault,
   className,
@@ -48,31 +32,8 @@ export const ChartSettingInputNumeric = ({
   const [inputValue, setInputValue] = useState<string>(value?.toString() ?? "");
   const defaultValueProps = getDefault ? { defaultValue: getDefault() } : {};
 
-  const handleChangeRef = useLatest((e: ChangeEvent<HTMLInputElement>) => {
-    let num = e.target.value !== "" ? Number(e.target.value) : Number.NaN;
-    if (options?.isInteger) {
-      num = Math.round(num);
-    }
-    if (options?.isNonNegative && num < 0) {
-      num *= -1;
-    }
-
-    if (isNaN(num)) {
-      onChange(undefined);
-    } else {
-      onChange(num);
-      setInputValue(String(num));
-    }
-  });
-  const handleChangeDebounced = useMemo(() => {
-    return debounce((e: ChangeEvent<HTMLInputElement>) => {
-      handleChangeRef.current(e);
-    }, 400);
-  }, [handleChangeRef]);
-
-  useEffect(() => {
-    setInputValue(value?.toString() ?? "");
-  }, [value]);
+  const num = parseNumber(inputValue, options);
+  const isValid = inputValue === undefined || inputValue === "" || !isNaN(num);
 
   return (
     <TextInput
@@ -80,15 +41,38 @@ export const ChartSettingInputNumeric = ({
       {...defaultValueProps}
       placeholder={placeholder}
       type="text"
-      error={inputValue && isNaN(Number(inputValue))}
+      error={!isValid}
       value={inputValue}
-      onChange={(e: ChangeEvent<HTMLInputElement>) => {
-        if (e.target.value.split("").every((ch) => ALLOWED_CHARS.has(ch))) {
-          setInputValue(e.target.value);
-          handleChangeDebounced(e);
+      onChange={(event) => setInputValue(event.target.value)}
+      onBlur={(event) => {
+        const num = parseNumber(event.target.value, options);
+        if (!isNaN(num)) {
+          onChange(num);
+
+          // Only change the text in the input if the value actually changed
+          if (num !== parseFloat(inputValue)) {
+            setInputValue(num.toString());
+          }
+        } else {
+          onChange(undefined);
+          setInputValue("");
         }
       }}
       className={className}
     />
   );
 };
+
+function parseNumber(value: string, options: NumberOptions) {
+  let num = value !== "" ? Number(value) : Number.NaN;
+
+  if (options?.isInteger) {
+    num = Math.round(num);
+  }
+
+  if (options?.isNonNegative && num < 0) {
+    num *= -1;
+  }
+
+  return num;
+}

frontend/src/metabase/visualizations/components/settings/ChartSettingInputNumeric.unit.spec.tsx

@@ -1,17 +1,9 @@
 import userEvent from "@testing-library/user-event";
 
-import { act, renderWithProviders, screen } from "__support__/ui";
+import { fireEvent, renderWithProviders, screen } from "__support__/ui";
 
 import { ChartSettingInputNumeric } from "./ChartSettingInputNumeric";
 
-beforeEach(() => {
-  jest.useFakeTimers();
-});
-
-afterEach(() => {
-  jest.useRealTimers();
-});
-
 function setup({
   value,
   options,
@@ -38,10 +30,9 @@ function setup({
 }
 
 async function type({ input, value }: { input: HTMLElement; value: string }) {
-  const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });
-  await user.clear(input);
-  await user.type(input, value);
-  act(() => jest.runAllTimers());
+  await userEvent.clear(input);
+  await userEvent.type(input, value);
+  fireEvent.blur(input);
 }
 
 describe("ChartSettingInputNumber", () => {
@@ -71,7 +62,7 @@ describe("ChartSettingInputNumber", () => {
     // multiple decimal places should call onChange with
     // undefined since it's an invalid value
     await type({ input, value: "1.2.3" });
-    expect(input).toHaveDisplayValue("1.2.3");
+    expect(input).toHaveDisplayValue("");
     expect(onChange).toHaveBeenCalledWith(undefined);
   });
 
@@ -79,7 +70,7 @@ describe("ChartSettingInputNumber", () => {
     const { input, onChange } = setup();
 
     await type({ input, value: "1.5e3" });
-    expect(input).toHaveDisplayValue("1500");
+    expect(input).toHaveDisplayValue("1.5e3");
     expect(onChange).toHaveBeenCalledWith(1.5e3);
   });
 
@@ -126,24 +117,24 @@ describe("ChartSettingInputNumber", () => {
 
     await type({ input, value: "asdf" });
     expect(input).toHaveDisplayValue("");
-    expect(onChange).not.toHaveBeenCalled();
+    expect(onChange).toHaveBeenCalledWith(undefined);
 
     // Inputs with `e` that are not valid scientific notation
     type({ input, value: "e123" });
     expect(input).toHaveDisplayValue("");
-    expect(onChange).not.toHaveBeenCalled();
+    expect(onChange).toHaveBeenCalledWith(undefined);
 
     type({ input, value: "e123e" });
     expect(input).toHaveDisplayValue("");
-    expect(onChange).not.toHaveBeenCalled();
+    expect(onChange).toHaveBeenCalledWith(undefined);
 
     type({ input, value: "1e23e" });
     expect(input).toHaveDisplayValue("");
-    expect(onChange).not.toHaveBeenCalled();
+    expect(onChange).toHaveBeenCalledWith(undefined);
 
     type({ input, value: "e1e23e" });
     expect(input).toHaveDisplayValue("");
-    expect(onChange).not.toHaveBeenCalled();
+    expect(onChange).toHaveBeenCalledWith(undefined);
   });
 
   it("renders the `value` prop on load", () => {