From 261a5ef50546730d2a25e9a75008b0ae6181e530 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 6 Oct 2023 14:29:11 +0000 Subject: [PATCH 1/7] fix: make error text less naggy --- .../Dialogs/DeleteDialog/DeleteDialog.tsx | 122 ++++++++++-------- 1 file changed, 65 insertions(+), 57 deletions(-) diff --git a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx index 89fef1df95fe1..9ab5ae5971b72 100644 --- a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx +++ b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx @@ -1,6 +1,13 @@ -import makeStyles from "@mui/styles/makeStyles"; +import { + type FC, + type FormEvent, + type PropsWithChildren, + useId, + useState, +} from "react"; + +import { useTheme } from "@emotion/react"; import TextField from "@mui/material/TextField"; -import { ChangeEvent, useState, PropsWithChildren, FC } from "react"; import { ConfirmDialog } from "../ConfirmDialog/ConfirmDialog"; export interface DeleteDialogProps { @@ -22,51 +29,23 @@ export const DeleteDialog: FC> = ({ name, confirmLoading, }) => { - const styles = useStyles(); - const [nameValue, setNameValue] = useState(""); - const confirmed = name === nameValue; - const handleChange = (event: ChangeEvent) => { - setNameValue(event.target.value); - }; - const hasError = nameValue.length > 0 && !confirmed; + const hookId = useId(); + const theme = useTheme(); - const content = ( - <> -

Deleting this {entity} is irreversible!

- {Boolean(info) &&

{info}

} -

Are you sure you want to proceed?

-

- Type “{name}” below to confirm. -

+ const [confirmationText, setConfirmationText] = useState(""); + const [isFocused, setIsFocused] = useState(false); -
{ - e.preventDefault(); - if (confirmed) { - onConfirm(); - } - }} - > - - - - ); + const onSubmit = (e: FormEvent) => { + e.preventDefault(); + if (deletionConfirmed) { + onConfirm(); + } + }; + + const confirmationId = `${hookId}-${DeleteDialog.name}-confirm`; + const deletionConfirmed = name === confirmationText; + const hasError = + !isFocused && !deletionConfirmed && confirmationText.length > 0; return ( > = ({ title={`Delete ${entity}`} onConfirm={onConfirm} onClose={onCancel} - description={content} confirmLoading={confirmLoading} - disabled={!confirmed} + disabled={!deletionConfirmed} + description={ + <> +

Deleting this {entity} is irreversible!

+ + {Boolean(info) && ( +

{info}

+ )} + +

Are you sure you want to proceed?

+ +

+ Type “{name}” below to confirm. +

+ +
+ setConfirmationText(event.target.value)} + onFocus={() => setIsFocused(true)} + onBlur={() => setIsFocused(false)} + label={`Name of the ${entity} to delete`} + error={hasError} + helperText={ + hasError && + `${confirmationText} does not match the name of this ${entity}` + } + inputProps={{ + ["data-testid"]: "delete-dialog-name-confirmation", + }} + /> + + + } /> ); }; - -const useStyles = makeStyles((theme) => ({ - warning: { - color: theme.palette.warning.light, - }, - - textField: { - marginTop: theme.spacing(3), - }, -})); From 3fbc94dcb58d55ce9d5f9d1c16404e8226b12d7a Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 6 Oct 2023 14:44:10 +0000 Subject: [PATCH 2/7] fix: make input colors sync with confirmation text state --- .../Dialogs/DeleteDialog/DeleteDialog.tsx | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx index 9ab5ae5971b72..60254ea394571 100644 --- a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx +++ b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx @@ -35,17 +35,18 @@ export const DeleteDialog: FC> = ({ const [confirmationText, setConfirmationText] = useState(""); const [isFocused, setIsFocused] = useState(false); - const onSubmit = (e: FormEvent) => { - e.preventDefault(); + const deletionConfirmed = name === confirmationText; + const onSubmit = (event: FormEvent) => { + event.preventDefault(); if (deletionConfirmed) { onConfirm(); } }; const confirmationId = `${hookId}-${DeleteDialog.name}-confirm`; - const deletionConfirmed = name === confirmationText; - const hasError = - !isFocused && !deletionConfirmed && confirmationText.length > 0; + const hasError = !deletionConfirmed && confirmationText.length > 0; + const displayErrorMessage = hasError && !isFocused; + const color = hasError ? "error" : "primary"; return ( > = ({ onFocus={() => setIsFocused(true)} onBlur={() => setIsFocused(false)} label={`Name of the ${entity} to delete`} - error={hasError} + color={color} + error={displayErrorMessage} helperText={ - hasError && + displayErrorMessage && `${confirmationText} does not match the name of this ${entity}` } + InputProps={{ color }} inputProps={{ ["data-testid"]: "delete-dialog-name-confirmation", }} From f6cee9aeadba0382de0fb2389bcdc0f6e1fe8134 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 6 Oct 2023 14:45:25 +0000 Subject: [PATCH 3/7] fix: more color sync fixes --- site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx index 60254ea394571..6529edbba302e 100644 --- a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx +++ b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx @@ -46,7 +46,7 @@ export const DeleteDialog: FC> = ({ const confirmationId = `${hookId}-${DeleteDialog.name}-confirm`; const hasError = !deletionConfirmed && confirmationText.length > 0; const displayErrorMessage = hasError && !isFocused; - const color = hasError ? "error" : "primary"; + const inputColor = hasError ? "error" : "primary"; return ( > = ({ onFocus={() => setIsFocused(true)} onBlur={() => setIsFocused(false)} label={`Name of the ${entity} to delete`} - color={color} + color={inputColor} error={displayErrorMessage} helperText={ displayErrorMessage && `${confirmationText} does not match the name of this ${entity}` } - InputProps={{ color }} + InputProps={{ color: inputColor }} inputProps={{ ["data-testid"]: "delete-dialog-name-confirmation", }} From 1060cc025c65a9e7ba009d10f9642bc5ed72bfaf Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 6 Oct 2023 16:21:40 +0000 Subject: [PATCH 4/7] fix: remove flaky warning messages in test --- .../DeleteDialog/DeleteDialog.test.tsx | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.test.tsx b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.test.tsx index c88bbea814f03..54cfda56598c7 100644 --- a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.test.tsx +++ b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.test.tsx @@ -2,6 +2,21 @@ import { screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { render } from "testHelpers/renderHelpers"; import { DeleteDialog } from "./DeleteDialog"; +import { act } from "react-dom/test-utils"; + +const inputTestId = "delete-dialog-name-confirmation"; + +async function fillInputField(inputElement: HTMLElement, text: string) { + // 2023-10-06 - There's something wonky with MUI's ConfirmDialog that causes + // its state to update after a typing event being fired, and React Testing + // Library isn't able to catch it, making React DOM freak out because an + // "unexpected" state change happened. Tried everything under the sun to catch + // the state changes the proper way, but the only way to get around it for now + // might be to manually make React aware of the changes + + // eslint-disable-next-line testing-library/no-unnecessary-act -- have to make sure state updates don't slip through cracks + return act(() => userEvent.type(inputElement, text)); +} describe("DeleteDialog", () => { it("disables confirm button when the text field is empty", () => { @@ -14,6 +29,7 @@ describe("DeleteDialog", () => { name="MyTemplate" />, ); + const confirmButton = screen.getByRole("button", { name: "Delete" }); expect(confirmButton).toBeDisabled(); }); @@ -28,8 +44,10 @@ describe("DeleteDialog", () => { name="MyTemplate" />, ); - const textField = screen.getByTestId("delete-dialog-name-confirmation"); - await userEvent.type(textField, "MyTemplateWrong"); + + const textField = screen.getByTestId(inputTestId); + await fillInputField(textField, "MyTemplateButWrong"); + const confirmButton = screen.getByRole("button", { name: "Delete" }); expect(confirmButton).toBeDisabled(); }); @@ -44,8 +62,10 @@ describe("DeleteDialog", () => { name="MyTemplate" />, ); - const textField = screen.getByTestId("delete-dialog-name-confirmation"); - await userEvent.type(textField, "MyTemplate"); + + const textField = screen.getByTestId(inputTestId); + await fillInputField(textField, "MyTemplate"); + const confirmButton = screen.getByRole("button", { name: "Delete" }); expect(confirmButton).not.toBeDisabled(); }); From 2957267cd96e4d22c638bb58f508dc52b546505e Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 6 Oct 2023 16:37:03 +0000 Subject: [PATCH 5/7] fix: remove needless braces --- site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx index 6529edbba302e..3d71363fa0865 100644 --- a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx +++ b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx @@ -94,7 +94,7 @@ export const DeleteDialog: FC> = ({ } InputProps={{ color: inputColor }} inputProps={{ - ["data-testid"]: "delete-dialog-name-confirmation", + "data-testid": "delete-dialog-name-confirmation", }} /> From e10d8f5b223629c12aacd958ccb198ccd98b7f98 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 6 Oct 2023 19:14:45 +0000 Subject: [PATCH 6/7] refactor: clean up code --- .../Dialogs/DeleteDialog/DeleteDialog.test.tsx | 10 ++++++---- .../components/Dialogs/DeleteDialog/DeleteDialog.tsx | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.test.tsx b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.test.tsx index 54cfda56598c7..de01e14cc4419 100644 --- a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.test.tsx +++ b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.test.tsx @@ -8,11 +8,13 @@ const inputTestId = "delete-dialog-name-confirmation"; async function fillInputField(inputElement: HTMLElement, text: string) { // 2023-10-06 - There's something wonky with MUI's ConfirmDialog that causes - // its state to update after a typing event being fired, and React Testing + // its state to update after a typing event gets fired, and React Testing // Library isn't able to catch it, making React DOM freak out because an - // "unexpected" state change happened. Tried everything under the sun to catch - // the state changes the proper way, but the only way to get around it for now - // might be to manually make React aware of the changes + // "unexpected" state change happened. It won't fail the test, but it makes + // the console look really scary because it'll spit out a big warning message. + // Tried everything under the sun to catch the state changes the proper way, + // but the only way to get around it for now might be to manually make React + // DOM aware of the changes // eslint-disable-next-line testing-library/no-unnecessary-act -- have to make sure state updates don't slip through cracks return act(() => userEvent.type(inputElement, text)); diff --git a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx index 3d71363fa0865..5c08d50d55a02 100644 --- a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx +++ b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx @@ -43,7 +43,7 @@ export const DeleteDialog: FC> = ({ } }; - const confirmationId = `${hookId}-${DeleteDialog.name}-confirm`; + const confirmationId = `${hookId}-confirm`; const hasError = !deletionConfirmed && confirmationText.length > 0; const displayErrorMessage = hasError && !isFocused; const inputColor = hasError ? "error" : "primary"; From 13dc991ca9f5a580f01e739b2963031b14caedb0 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 6 Oct 2023 19:24:41 +0000 Subject: [PATCH 7/7] refactor: clean up code more --- .../Dialogs/DeleteDialog/DeleteDialog.tsx | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx index 5c08d50d55a02..ab1134bb6a397 100644 --- a/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx +++ b/site/src/components/Dialogs/DeleteDialog/DeleteDialog.tsx @@ -32,10 +32,10 @@ export const DeleteDialog: FC> = ({ const hookId = useId(); const theme = useTheme(); - const [confirmationText, setConfirmationText] = useState(""); + const [userConfirmationText, setUserConfirmationText] = useState(""); const [isFocused, setIsFocused] = useState(false); - const deletionConfirmed = name === confirmationText; + const deletionConfirmed = name === userConfirmationText; const onSubmit = (event: FormEvent) => { event.preventDefault(); if (deletionConfirmed) { @@ -43,8 +43,7 @@ export const DeleteDialog: FC> = ({ } }; - const confirmationId = `${hookId}-confirm`; - const hasError = !deletionConfirmed && confirmationText.length > 0; + const hasError = !deletionConfirmed && userConfirmationText.length > 0; const displayErrorMessage = hasError && !isFocused; const inputColor = hasError ? "error" : "primary"; @@ -79,10 +78,10 @@ export const DeleteDialog: FC> = ({ sx={{ marginTop: theme.spacing(3) }} name="confirmation" autoComplete="off" - id={confirmationId} + id={`${hookId}-confirm`} placeholder={name} - value={confirmationText} - onChange={(event) => setConfirmationText(event.target.value)} + value={userConfirmationText} + onChange={(event) => setUserConfirmationText(event.target.value)} onFocus={() => setIsFocused(true)} onBlur={() => setIsFocused(false)} label={`Name of the ${entity} to delete`} @@ -90,7 +89,7 @@ export const DeleteDialog: FC> = ({ error={displayErrorMessage} helperText={ displayErrorMessage && - `${confirmationText} does not match the name of this ${entity}` + `${userConfirmationText} does not match the name of this ${entity}` } InputProps={{ color: inputColor }} inputProps={{