From 10a094bee1a148e61604f27c95f865d8fa72799c Mon Sep 17 00:00:00 2001 From: Vladimir Cucu <108150922+vladimir-cucu@users.noreply.github.com> Date: Tue, 26 Mar 2024 08:11:21 +0200 Subject: [PATCH 1/9] fix: stop click event propagation in Modal (#1058) --- .../ConfirmationModal.test.tsx | 69 +++++++++++++++++++ .../ConfirmationModal/ConfirmationModal.tsx | 20 +++++- src/components/Modal/Modal.test.tsx | 32 +++++++++ src/components/Modal/Modal.tsx | 58 ++++++++++------ 4 files changed, 156 insertions(+), 23 deletions(-) diff --git a/src/components/ConfirmationModal/ConfirmationModal.test.tsx b/src/components/ConfirmationModal/ConfirmationModal.test.tsx index a984f374d..73830c53b 100644 --- a/src/components/ConfirmationModal/ConfirmationModal.test.tsx +++ b/src/components/ConfirmationModal/ConfirmationModal.test.tsx @@ -55,4 +55,73 @@ describe("ConfirmationModal ", () => { await userEvent.click(screen.getByText("Proceed")); expect(onConfirm).toHaveBeenCalled(); }); + + it("should stop click event propagation on cancel by default", async () => { + const handleExternalClick = jest.fn(); + render( +
+ + Test click propagation + +
+ ); + + await userEvent.click(screen.getByText("Go back")); + expect(handleExternalClick).not.toHaveBeenCalled(); + }); + + it("should propagate click event on cancel", async () => { + const handleExternalClick = jest.fn(); + render( +
+ + Test click propagation + +
+ ); + + await userEvent.click(screen.getByText("Go back")); + expect(handleExternalClick).toHaveBeenCalledTimes(1); + }); + + it("should stop click event propagation on confirm by default", async () => { + const handleExternalClick = jest.fn(); + render( +
+ + Test click propagation + +
+ ); + + await userEvent.click(screen.getByText("Proceed")); + expect(handleExternalClick).not.toHaveBeenCalled(); + }); + + it("should propagate click event on confirm", async () => { + const handleExternalClick = jest.fn(); + render( +
+ + Test click propagation + +
+ ); + + await userEvent.click(screen.getByText("Proceed")); + expect(handleExternalClick).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/components/ConfirmationModal/ConfirmationModal.tsx b/src/components/ConfirmationModal/ConfirmationModal.tsx index b647790c1..41edfe55e 100644 --- a/src/components/ConfirmationModal/ConfirmationModal.tsx +++ b/src/components/ConfirmationModal/ConfirmationModal.tsx @@ -29,7 +29,7 @@ export type Props = PropsWithSpread< /** * Function to perform the action prompted by the modal. */ - onConfirm: (e: MouseEvent) => void; + onConfirm: (event: MouseEvent) => void; }, Omit >; @@ -43,18 +43,32 @@ export const ConfirmationModal = ({ onConfirm, ...props }: Props): ReactElement => { + const handleClick = + (action: A | null | undefined) => + (event: MouseEvent) => { + if (!props.shouldPropagateClickEvent) { + event.stopPropagation(); + } + if (action) { + action(event); + } + }; + return ( {confirmExtra} - diff --git a/src/components/Modal/Modal.test.tsx b/src/components/Modal/Modal.test.tsx index e6a61eb25..b596f1af8 100644 --- a/src/components/Modal/Modal.test.tsx +++ b/src/components/Modal/Modal.test.tsx @@ -139,4 +139,36 @@ describe("Modal ", () => { expect(mockHandleModalClose).toHaveBeenCalledTimes(1); expect(mockHandleEscPress).not.toHaveBeenCalled(); }); + + it("should stop click event propagation on close by default", async () => { + const handleExternalClick = jest.fn(); + const { container } = render( +
+ + Bare bones + +
+ ); + + const closeButton = container.querySelector("button.p-modal__close"); + expect(closeButton).not.toBeNull(); + await userEvent.click(closeButton!); + expect(handleExternalClick).not.toHaveBeenCalled(); + }); + + it("should propagate click event on close", async () => { + const handleExternalClick = jest.fn(); + const { container } = render( +
+ + Bare bones + +
+ ); + + const closeButton = container.querySelector("button.p-modal__close"); + expect(closeButton).not.toBeNull(); + await userEvent.click(closeButton!); + expect(handleExternalClick).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/components/Modal/Modal.tsx b/src/components/Modal/Modal.tsx index 5f66803e1..231347b4b 100644 --- a/src/components/Modal/Modal.tsx +++ b/src/components/Modal/Modal.tsx @@ -25,6 +25,10 @@ export type Props = PropsWithSpread< * The title of the modal. */ title?: ReactNode | null; + /** + * Whether the button click event should propagate. + */ + shouldPropagateClickEvent?: boolean; }, HTMLProps >; @@ -35,6 +39,7 @@ export const Modal = ({ className, close, title, + shouldPropagateClickEvent = false, ...wrapperProps }: Props): ReactElement => { // list of focusable selectors is based on this Stack Overflow answer: @@ -47,7 +52,7 @@ export const Modal = ({ const modalRef: MutableRefObject = useRef(null); const focusableModalElements = useRef(null); - const handleTabKey = (e: React.KeyboardEvent) => { + const handleTabKey = (event: React.KeyboardEvent) => { if (focusableModalElements.current.length > 0) { const firstElement = focusableModalElements.current[0]; const lastElement = @@ -55,29 +60,31 @@ export const Modal = ({ focusableModalElements.current.length - 1 ]; - if (!e.shiftKey && document.activeElement === lastElement) { + if (!event.shiftKey && document.activeElement === lastElement) { (firstElement as HTMLElement).focus(); - e.preventDefault(); + event.preventDefault(); } - if (e.shiftKey && document.activeElement === firstElement) { + if (event.shiftKey && document.activeElement === firstElement) { (lastElement as HTMLElement).focus(); - return e.preventDefault(); + return event.preventDefault(); } } }; const handleEscKey = ( - e: KeyboardEvent | React.KeyboardEvent + event: KeyboardEvent | React.KeyboardEvent ) => { - if ("nativeEvent" in e && e.nativeEvent.stopImmediatePropagation) { - e.nativeEvent.stopImmediatePropagation(); - } else if ("stopImmediatePropagation" in e) { - e.stopImmediatePropagation(); - } else if (e.stopPropagation) { - e.stopPropagation(); + if ("nativeEvent" in event && event.nativeEvent.stopImmediatePropagation) { + event.nativeEvent.stopImmediatePropagation(); + } else if ("stopImmediatePropagation" in event) { + event.stopImmediatePropagation(); + } else if (event.stopPropagation) { + event.stopPropagation(); + } + if (close) { + close(); } - close(); }; const keyListenersMap = new Map([ @@ -102,9 +109,9 @@ export const Modal = ({ }, [close]); useEffect(() => { - const keyDown = (e) => { - const listener = keyListenersMap.get(e.code); - return listener && listener(e); + const keyDown = (event: KeyboardEvent) => { + const listener = keyListenersMap.get(event.code); + return listener && listener(event); }; document.addEventListener("keydown", keyDown); @@ -121,18 +128,29 @@ export const Modal = ({ shouldClose.current = false; }; - const handleOverlayOnMouseDown = (event) => { + const handleOverlayOnMouseDown = ( + event: React.MouseEvent + ) => { if (event.target === modalRef.current) { shouldClose.current = true; } }; - const handleOverlayOnClick = () => { - if (shouldClose.current) { + const handleClose = (event: React.MouseEvent) => { + if (!shouldPropagateClickEvent) { + event.stopPropagation(); + } + if (close) { close(); } }; + const handleOverlayOnClick = (event: React.MouseEvent) => { + if (shouldClose.current) { + handleClose(event); + } + }; + return (
Close From 55da75d951d14e96ffcf33df439121ba0cc0cf94 Mon Sep 17 00:00:00 2001 From: David Edler Date: Wed, 3 Apr 2024 17:06:05 +0200 Subject: [PATCH 2/9] fix: keep tooltip open, when it wraps multiple children and we change which child ishovered Signed-off-by: David Edler --- src/components/Tooltip/Tooltip.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/components/Tooltip/Tooltip.tsx b/src/components/Tooltip/Tooltip.tsx index 62756504f..66e0eb17b 100644 --- a/src/components/Tooltip/Tooltip.tsx +++ b/src/components/Tooltip/Tooltip.tsx @@ -287,7 +287,8 @@ const Tooltip = ({ if ( e.relatedTarget - ? !messageRef.current?.contains(e.relatedTarget) + ? !messageRef.current?.contains(e.relatedTarget) && + !wrapperRef.current?.contains(e.relatedTarget) : e.target !== messageRef.current ) { cancelableClosePortal(); From 1f0ae65acf517c975e71040c5fa0a57d4dec0e0c Mon Sep 17 00:00:00 2001 From: Bartek Szopka Date: Thu, 21 Mar 2024 09:26:44 +0100 Subject: [PATCH 3/9] Require the review of Percy visual tests --- .github/labeler.yml | 3 +++ .github/workflows/pr-labels.yml | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 .github/workflows/pr-labels.yml diff --git a/.github/labeler.yml b/.github/labeler.yml index 8518bf75f..51c1dbefc 100644 --- a/.github/labeler.yml +++ b/.github/labeler.yml @@ -3,3 +3,6 @@ Maintenance 🔨: - any-glob-to-any-file: - package.json - .gitignore +"Review: Percy needed": +- changed-files: + - any-glob-to-any-file: src/** diff --git a/.github/workflows/pr-labels.yml b/.github/workflows/pr-labels.yml new file mode 100644 index 000000000..4baa2acc4 --- /dev/null +++ b/.github/workflows/pr-labels.yml @@ -0,0 +1,23 @@ +name: PR Percy Review Label Required +on: + pull_request: + types: [opened, labeled, unlabeled, synchronize] +jobs: + label: + runs-on: ubuntu-latest + permissions: + issues: write + pull-requests: write + steps: + - uses: mheap/github-action-required-labels@v5 + with: + mode: exactly + count: 1 + labels: "Review: Percy +1" + add_comment: true + message: | + This PR is being prevented from merging because it needs to be reviewed on Percy. + + Go to [Percy](https://percy.io/bb49709b/react-components), find the build relevant to this PR and check if it looks as expected. + + Once it's approved, add the label `Review: Percy +1` to this PR. From ef320d93ec54826eacb52ee9e5b23548738b6b4b Mon Sep 17 00:00:00 2001 From: lorumic Date: Thu, 4 Apr 2024 17:20:34 +0200 Subject: [PATCH 4/9] fix(field): remove bold prefix for error/caution/success validation --- src/components/Field/Field.test.tsx | 12 ++++++------ src/components/Field/Field.tsx | 4 +--- src/components/FormikField/FormikField.test.tsx | 4 +--- src/components/Input/Input.test.tsx | 12 +++--------- .../PasswordToggle/PasswordToggle.test.tsx | 2 +- src/components/Select/Select.test.tsx | 4 +--- src/components/Slider/Slider.test.tsx | 4 +--- src/components/Textarea/Textarea.test.tsx | 4 +--- 8 files changed, 15 insertions(+), 31 deletions(-) diff --git a/src/components/Field/Field.test.tsx b/src/components/Field/Field.test.tsx index f0e7e81d6..b7550b373 100644 --- a/src/components/Field/Field.test.tsx +++ b/src/components/Field/Field.test.tsx @@ -27,7 +27,7 @@ describe("Field ", () => { ); expect(screen.getByRole("textbox")).toHaveAccessibleErrorMessage( - "Caution: Are you sure?" + "Are you sure?" ); expect(screen.getByTestId("field")).toHaveClass("is-caution"); }); @@ -43,7 +43,7 @@ describe("Field ", () => { ); expect(screen.getByRole("textbox")).toHaveAccessibleErrorMessage( - "Caution: Are you sure?" + "Are you sure?" ); expect(screen.getByTestId("field")).toHaveClass("is-caution"); }); @@ -55,7 +55,7 @@ describe("Field ", () => { ); expect(screen.getByRole("textbox")).toHaveAccessibleErrorMessage( - "Error: You can't do that" + "You can't do that" ); expect(screen.getByTestId("field")).toHaveClass("is-error"); }); @@ -71,7 +71,7 @@ describe("Field ", () => { ); expect(screen.getByRole("textbox")).toHaveAccessibleErrorMessage( - "Error: You can't do that" + "You can't do that" ); expect(screen.getByTestId("field")).toHaveClass("is-error"); }); @@ -83,7 +83,7 @@ describe("Field ", () => { ); expect(screen.getByRole("textbox")).toHaveAccessibleDescription( - "Success: You did it!" + "You did it!" ); expect(screen.getByTestId("field")).toHaveClass("is-success"); }); @@ -99,7 +99,7 @@ describe("Field ", () => { ); expect(screen.getByRole("textbox")).toHaveAccessibleDescription( - "Success: You did it!" + "You did it!" ); expect(screen.getByTestId("field")).toHaveClass("is-success"); }); diff --git a/src/components/Field/Field.tsx b/src/components/Field/Field.tsx index e983eba10..21a9d7bb3 100644 --- a/src/components/Field/Field.tsx +++ b/src/components/Field/Field.tsx @@ -107,11 +107,9 @@ const generateError = ( if (!error && !caution && !success) { return null; } - const messageType = - (error && "Error") || (caution && "Caution") || (success && "Success"); return (

- {messageType}: {error || caution || success} + {error || caution || success}

); }; diff --git a/src/components/FormikField/FormikField.test.tsx b/src/components/FormikField/FormikField.test.tsx index 9b4ab1ad1..75da14fc0 100644 --- a/src/components/FormikField/FormikField.test.tsx +++ b/src/components/FormikField/FormikField.test.tsx @@ -29,9 +29,7 @@ describe("FormikField", () => { ); - expect(screen.getByRole("textbox")).toHaveAccessibleErrorMessage( - "Error: Uh oh!" - ); + expect(screen.getByRole("textbox")).toHaveAccessibleErrorMessage("Uh oh!"); }); it("can hide the errors", () => { diff --git a/src/components/Input/Input.test.tsx b/src/components/Input/Input.test.tsx index 9ba21d0d8..11ea7b910 100644 --- a/src/components/Input/Input.test.tsx +++ b/src/components/Input/Input.test.tsx @@ -103,23 +103,17 @@ describe("Input", () => { it("can display an error for a text input", async () => { render(); - expect(screen.getByRole("textbox")).toHaveAccessibleErrorMessage( - "Error: Uh oh!" - ); + expect(screen.getByRole("textbox")).toHaveAccessibleErrorMessage("Uh oh!"); }); it("can display an error for a radiobutton", async () => { render(); - expect(screen.getByRole("radio")).toHaveAccessibleErrorMessage( - "Error: Uh oh!" - ); + expect(screen.getByRole("radio")).toHaveAccessibleErrorMessage("Uh oh!"); }); it("can display an error for a checkbox", async () => { render(); - expect(screen.getByRole("checkbox")).toHaveAccessibleErrorMessage( - "Error: Uh oh!" - ); + expect(screen.getByRole("checkbox")).toHaveAccessibleErrorMessage("Uh oh!"); }); it("can display help for a text input", async () => { diff --git a/src/components/PasswordToggle/PasswordToggle.test.tsx b/src/components/PasswordToggle/PasswordToggle.test.tsx index 88fda3560..0df83024e 100644 --- a/src/components/PasswordToggle/PasswordToggle.test.tsx +++ b/src/components/PasswordToggle/PasswordToggle.test.tsx @@ -50,7 +50,7 @@ describe("PasswordToggle", () => { it("can display an error", async () => { render(); expect(screen.getByLabelText("password")).toHaveAccessibleErrorMessage( - "Error: Uh oh!" + "Uh oh!" ); }); diff --git a/src/components/Select/Select.test.tsx b/src/components/Select/Select.test.tsx index f5ea891b7..0d11cee02 100644 --- a/src/components/Select/Select.test.tsx +++ b/src/components/Select/Select.test.tsx @@ -55,9 +55,7 @@ describe("Select", () => { it("can display an error", async () => { render(