[UI v2] Fix tab order in variable create dialog (PrefectHQ#16014)

mthatt · Dec 17, 2024 · d3e43d4 · d3e43d4
1 parent bae6a76
commit d3e43d4
Show file tree

Hide file tree

Showing 4 changed files with 77 additions and 34 deletions.
diff --git a/ui-v2/src/components/ui/json-input.tsx b/ui-v2/src/components/ui/json-input.tsx
@@ -0,0 +1,59 @@
+import React, { useRef, useEffect } from "react";
+
+import { json } from "@codemirror/lang-json";
+import { cn } from "@/lib/utils";
+import { useCodeMirror } from "@uiw/react-codemirror";
+
+const extensions = [json()];
+
+type JsonInputProps = React.ComponentProps<"div"> & {
+	value?: string;
+	onChange?: (value: string) => void;
+	onBlur?: () => void;
+	disabled?: boolean;
+	className?: string;
+};
+
+export const JsonInput = React.forwardRef<HTMLDivElement, JsonInputProps>(
+	(
+		{ className, value, onChange, onBlur, disabled, ...props },
+		forwardedRef,
+	) => {
+		const editor = useRef<HTMLDivElement | null>(null);
+		const { setContainer } = useCodeMirror({
+			container: editor.current,
+			extensions,
+			value,
+			onChange,
+			onBlur,
+			indentWithTab: false,
+			editable: !disabled,
+		});
+
+		useEffect(() => {
+			if (editor.current) {
+				setContainer(editor.current);
+			}
+		}, [setContainer]);
+
+		return (
+			<div
+				className={cn(
+					"rounded-md border shadow-sm overflow-hidden focus-within:outline-none focus-within:ring-1 focus-within:ring-ring",
+					className,
+				)}
+				ref={(node) => {
+					editor.current = node;
+					if (typeof forwardedRef === "function") {
+						forwardedRef(node);
+					} else if (forwardedRef) {
+						forwardedRef.current = node;
+					}
+				}}
+				{...props}
+			/>
+		);
+	},
+);
+
+JsonInput.displayName = "JsonInput";
diff --git a/ui-v2/src/components/variables/add-variable-dialog.tsx b/ui-v2/src/components/variables/add-variable-dialog.tsx
@@ -9,8 +9,6 @@ import {
 	DialogTrigger,
 } from "@/components/ui/dialog";
 import { zodResolver } from "@hookform/resolvers/zod";
-import CodeMirror, { EditorView } from "@uiw/react-codemirror";
-import { json } from "@codemirror/lang-json";
 import { useMutation } from "@tanstack/react-query";
 import { useForm } from "react-hook-form";
 import { z } from "zod";
@@ -30,6 +28,7 @@ import { Loader2 } from "lucide-react";
 import { TagsInput } from "../ui/tags-input";
 import { useToast } from "@/hooks/use-toast";
 import { queryClient } from "@/router";
+import { JsonInput } from "@/components/ui/json-input";
 
 const formSchema = z.object({
 	name: z.string().min(2, { message: "Name must be at least 2 characters" }),
@@ -133,7 +132,7 @@ export const AddVariableDialog = ({
 								<FormItem>
 									<FormLabel>Name</FormLabel>
 									<FormControl>
-										<Input {...field} />
+										<Input autoComplete="off" {...field} />
 									</FormControl>
 									<FormMessage />
 								</FormItem>
@@ -144,23 +143,9 @@ export const AddVariableDialog = ({
 							name="value"
 							render={({ field }) => (
 								<FormItem>
-									<FormLabel id="value-label">Value</FormLabel>
+									<FormLabel>Value</FormLabel>
 									<FormControl>
-										<CodeMirror
-											aria-labelledby="value-label"
-											extensions={[json()]}
-											basicSetup={{
-												foldGutter: false,
-												history: false,
-											}}
-											theme={EditorView.theme({
-												"&.cm-editor.cm-focused": {
-													outline: "none",
-												},
-											})}
-											className="rounded-md border shadow-sm overflow-hidden"
-											{...field}
-										/>
+										<JsonInput {...field} />
 									</FormControl>
 									<FormMessage />
 								</FormItem>

diff --git a/ui-v2/tests/variables/mocks.tsx b/ui-v2/tests/variables/mocks.tsx
@@ -1,7 +1,10 @@
 import { vi } from "vitest";
 import React from "react";
 
-export const MockCodeMirror = React.forwardRef<
+//  Mock out JsonInput because the underlying CodeMirror editor
+//  relies on browser APIs that are not available in JSDOM.
+// TODO: Ensure input into JsonInput is covered by Playwright tests.
+export const MockJsonInput = React.forwardRef<
 	HTMLTextAreaElement,
 	{
 		value: string;
@@ -15,17 +18,13 @@ export const MockCodeMirror = React.forwardRef<
 			onChange={(e) => {
 				props.onChange?.(e.target.value);
 			}}
-			data-testid="mock-codemirror"
+			data-testid="mock-json-input"
 		/>
 	);
 });
 
-MockCodeMirror.displayName = "MockCodemirror";
+MockJsonInput.displayName = "MockJsonInput";
 
-vi.mock("@uiw/react-codemirror", () => ({
-	default: MockCodeMirror,
-	json: () => ({}),
-	EditorView: {
-		theme: () => ({}),
-	},
+vi.mock("@/components/ui/json-input", () => ({
+	JsonInput: MockJsonInput,
 }));
diff --git a/ui-v2/tests/variables/variables.test.tsx b/ui-v2/tests/variables/variables.test.tsx
@@ -70,14 +70,14 @@ describe("Variables page", () => {
 
 			await user.click(screen.getByRole("button", { name: "Add Variable" }));
 			await user.type(screen.getByLabelText("Name"), "my-variable");
-			await userEvent.type(screen.getByTestId("mock-codemirror"), "123");
+			await userEvent.type(screen.getByTestId("mock-json-input"), "123");
 			await user.click(
 				screen.getByRole("button", { name: "Close", expanded: true }),
 			);
 			await user.click(screen.getByRole("button", { name: "Add Variable" }));
 
 			expect(screen.getByLabelText("Name")).toHaveValue("");
-			expect(screen.getByTestId("mock-codemirror")).toHaveValue("");
+			expect(screen.getByTestId("mock-json-input")).toHaveValue("");
 		});
 
 		it("should allow adding a variable", async () => {
@@ -98,7 +98,7 @@ describe("Variables page", () => {
 			await user.click(screen.getByRole("button", { name: "Add Variable" }));
 			expect(screen.getByText("New Variable")).toBeVisible();
 			await user.type(screen.getByLabelText("Name"), "my-variable");
-			await userEvent.type(screen.getByTestId("mock-codemirror"), "123");
+			await userEvent.type(screen.getByTestId("mock-json-input"), "123");
 			await userEvent.type(screen.getByLabelText("Tags"), "tag1");
 			await user.click(screen.getByRole("button", { name: "Create" }));
 
@@ -129,7 +129,7 @@ describe("Variables page", () => {
 			// Value validation error
 			await user.type(screen.getByLabelText("Name"), "my-variable");
 			await userEvent.type(
-				screen.getByTestId("mock-codemirror"),
+				screen.getByTestId("mock-json-input"),
 				"{{Invalid JSON",
 			);
 			await user.click(screen.getByRole("button", { name: "Create" }));
@@ -160,7 +160,7 @@ describe("Variables page", () => {
 
 			await user.click(screen.getByRole("button", { name: "Add Variable" }));
 			await user.type(screen.getByLabelText("Name"), "my-variable");
-			await userEvent.type(screen.getByTestId("mock-codemirror"), "123");
+			await userEvent.type(screen.getByTestId("mock-json-input"), "123");
 			await user.click(screen.getByRole("button", { name: "Create" }));
 			expect(screen.getByText("Failed to create variable")).toBeVisible();
 		});
@@ -189,7 +189,7 @@ describe("Variables page", () => {
 
 			await user.click(screen.getByRole("button", { name: "Add Variable" }));
 			await user.type(screen.getByLabelText("Name"), "my-variable");
-			await userEvent.type(screen.getByTestId("mock-codemirror"), "123");
+			await userEvent.type(screen.getByTestId("mock-json-input"), "123");
 			await user.click(screen.getByRole("button", { name: "Create" }));
 
 			expect(