From 61577d01d001197c01e276fb9e52c9ccf3c5345b Mon Sep 17 00:00:00 2001
From: github-automation-metabase
<166700802+github-automation-metabase@users.noreply.github.com>
Date: Wed, 13 Nov 2024 01:34:16 -0500
Subject: [PATCH] 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>
---
.../components/LimitStep/LimitStep.tsx | 17 +++++++--
.../LimitStep/LimitStep.unit.spec.tsx | 36 +++++++++++++++++--
.../notebook/components/LimitStep/util.ts | 7 ++++
3 files changed, 55 insertions(+), 5 deletions(-)
create mode 100644 frontend/src/metabase/querying/notebook/components/LimitStep/util.ts
diff --git a/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.tsx b/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.tsx
index b0267f63065..6d1db67c6b6 100644
--- a/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.tsx
+++ b/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
diff --git a/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.unit.spec.tsx b/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.unit.spec.tsx
index 0016b27ae57..10643cdb80d 100644
--- a/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.unit.spec.tsx
+++ b/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();
+ });
});
diff --git a/frontend/src/metabase/querying/notebook/components/LimitStep/util.ts b/frontend/src/metabase/querying/notebook/components/LimitStep/util.ts
new file mode 100644
index 00000000000..d9e2f561aba
--- /dev/null
+++ b/frontend/src/metabase/querying/notebook/components/LimitStep/util.ts
@@ -0,0 +1,7 @@
+export function isLimitValid(number: number) {
+ return !Number.isNaN(number) && Number.isInteger(number) && number > 0;
+}
+
+export function parseLimit(value: string) {
+ return parseInt(value, 10);
+}
--
GitLab