Allow changing the limit using just the html input arrows (#49818) (#49873)

* Allow using the arrow buttons to change the limit value

* Add a min prop to the number input

* Use base-10



* Ensure number is integer

* Add issue name to unit test



---------

Co-authored-by: Romeo Van Snick <romeo@romeovansnick.be>
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>

frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.tsx

@@ -9,6 +9,8 @@ import * as Lib from "metabase-lib";
 import type { NotebookStepProps } from "../../types";
 import { NotebookCell } from "../NotebookCell";
 
+import { isLimitValid, parseLimit } from "./util";
+
 export function LimitStep({
   query,
   step,
@@ -20,15 +22,23 @@ export function LimitStep({
   const limit = Lib.currentLimit(query, stageIndex);
   const [value, setValue] = useState(typeof limit === "number" ? limit : "");
 
-  const handleBlur = (event: FocusEvent<HTMLInputElement>) => {
-    const nextLimit = parseInt(event.target.value, 0);
-    if (nextLimit >= 1) {
+  const updateLimit = (nextLimit: number) => {
+    if (isLimitValid(nextLimit)) {
       updateQuery(Lib.limit(query, stageIndex, nextLimit));
     }
   };
 
+  const handleBlur = (event: FocusEvent<HTMLInputElement>) => {
+    updateLimit(parseLimit(event.target.value));
+  };
+
   const handleChange = (event: ChangeEvent<HTMLInputElement>) => {
     setValue(event.target.value);
+
+    const isFocused = event.target === document.activeElement;
+    if (!isFocused) {
+      updateLimit(parseLimit(event.target.value));
+    }
   };
 
   return (
@@ -36,6 +46,7 @@ export function LimitStep({
       <LimitInput
         className={CS.mb1}
         type="number"
+        min={1}
         value={value}
         placeholder={t`Enter a limit`}
         small

frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.unit.spec.tsx

@@ -54,14 +54,36 @@ describe("LimitStep", () => {
     expect(Lib.currentLimit(getNextQuery(), 0)).toBe(52);
   });
 
-  it("should update the limit", () => {
+  it("should update the limit only when blurred", () => {
     const step = createMockNotebookStep({ query: QUERY_WITH_LIMIT });
-    const { getNextQuery } = setup(step);
+    const { getNextQuery, updateQuery } = setup(step);
 
     const limitInput = screen.getByPlaceholderText("Enter a limit");
+
+    fireEvent.focus(limitInput);
+
+    // HACK: manually set document.activeElement to the input
+    // so the focus check works.
+    Object.defineProperty(document, "activeElement", {
+      value: limitInput,
+      writable: false,
+    });
+
+    fireEvent.change(limitInput, { target: { value: "100" } });
     fireEvent.change(limitInput, { target: { value: "1000" } });
     fireEvent.blur(limitInput);
 
+    expect(updateQuery).toHaveBeenCalledTimes(1);
+    expect(Lib.currentLimit(getNextQuery(), 0)).toBe(1000);
+  });
+
+  it("should update the limit when not focused (eg. when clicking the arrows) (metabase#49587)", () => {
+    const step = createMockNotebookStep({ query: QUERY_WITH_LIMIT });
+    const { getNextQuery } = setup(step);
+
+    const limitInput = screen.getByPlaceholderText("Enter a limit");
+    fireEvent.change(limitInput, { target: { value: "1000" } });
+
     expect(Lib.currentLimit(getNextQuery(), 0)).toBe(1000);
   });
 
@@ -84,4 +106,14 @@ describe("LimitStep", () => {
 
     expect(updateQuery).not.toHaveBeenCalled();
   });
+
+  it("shouldn't update the limit if its not a valid number", () => {
+    const step = createMockNotebookStep({ query: QUERY_WITH_LIMIT });
+    const { updateQuery } = setup(step);
+
+    const limitInput = screen.getByPlaceholderText("Enter a limit");
+    fireEvent.change(limitInput, { target: { value: "not a number" } });
+
+    expect(updateQuery).not.toHaveBeenCalled();
+  });
 });

frontend/src/metabase/querying/notebook/components/LimitStep/util.ts

+export function isLimitValid(number: number) {
+  return !Number.isNaN(number) && Number.isInteger(number) && number > 0;
+}
+
+export function parseLimit(value: string) {
+  return parseInt(value, 10);
+}