From 7ca6900cbc527368c7d033caf18c1f97c5b461d4 Mon Sep 17 00:00:00 2001 From: vladimir-cucu Date: Sun, 24 Mar 2024 09:48:16 +0200 Subject: [PATCH 1/5] fix: stop click event propagation in Modal --- .../ConfirmationModal/ConfirmationModal.tsx | 18 +++++++++-- src/components/Modal/Modal.tsx | 30 ++++++++++++++----- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/components/ConfirmationModal/ConfirmationModal.tsx b/src/components/ConfirmationModal/ConfirmationModal.tsx index b647790c1..051ae8f10 100644 --- a/src/components/ConfirmationModal/ConfirmationModal.tsx +++ b/src/components/ConfirmationModal/ConfirmationModal.tsx @@ -43,18 +43,32 @@ export const ConfirmationModal = ({ onConfirm, ...props }: Props): ReactElement => { + const handleClick = + (action: Function | null | undefined) => + (e: MouseEvent) => { + if (!props.shouldPropagateClickEvent) { + e.stopPropagation(); + } + if (action) { + action(e); + } + }; + return ( {confirmExtra} - diff --git a/src/components/Modal/Modal.tsx b/src/components/Modal/Modal.tsx index 5f66803e1..c7306303a 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, ...wrapperProps }: Props): ReactElement => { // list of focusable selectors is based on this Stack Overflow answer: @@ -77,7 +82,9 @@ export const Modal = ({ } else if (e.stopPropagation) { e.stopPropagation(); } - close(); + if (close) { + close(); + } }; const keyListenersMap = new Map([ @@ -102,7 +109,7 @@ export const Modal = ({ }, [close]); useEffect(() => { - const keyDown = (e) => { + const keyDown = (e: KeyboardEvent) => { const listener = keyListenersMap.get(e.code); return listener && listener(e); }; @@ -121,18 +128,27 @@ export const Modal = ({ shouldClose.current = false; }; - const handleOverlayOnMouseDown = (event) => { - if (event.target === modalRef.current) { + const handleOverlayOnMouseDown = (e: React.MouseEvent) => { + if (e.target === modalRef.current) { shouldClose.current = true; } }; - const handleOverlayOnClick = () => { - if (shouldClose.current) { + const handleClose = (e: React.MouseEvent) => { + if (!shouldPropagateClickEvent) { + e.stopPropagation(); + } + if (close) { close(); } }; + const handleOverlayOnClick = (e: React.MouseEvent) => { + if (shouldClose.current) { + handleClose(e); + } + }; + return (
Close From edcaf497dabde3a0d8d8ea0a4e6f46556e80d0a9 Mon Sep 17 00:00:00 2001 From: vladimir-cucu Date: Mon, 25 Mar 2024 08:41:47 +0200 Subject: [PATCH 2/5] fix: resolve review comments --- .../ConfirmationModal/ConfirmationModal.tsx | 10 ++--- src/components/Modal/Modal.tsx | 44 ++++++++++--------- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/src/components/ConfirmationModal/ConfirmationModal.tsx b/src/components/ConfirmationModal/ConfirmationModal.tsx index 051ae8f10..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 >; @@ -44,13 +44,13 @@ export const ConfirmationModal = ({ ...props }: Props): ReactElement => { const handleClick = - (action: Function | null | undefined) => - (e: MouseEvent) => { + (action: A | null | undefined) => + (event: MouseEvent) => { if (!props.shouldPropagateClickEvent) { - e.stopPropagation(); + event.stopPropagation(); } if (action) { - action(e); + action(event); } }; diff --git a/src/components/Modal/Modal.tsx b/src/components/Modal/Modal.tsx index c7306303a..a33792241 100644 --- a/src/components/Modal/Modal.tsx +++ b/src/components/Modal/Modal.tsx @@ -52,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 = @@ -60,27 +60,27 @@ 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(); @@ -109,9 +109,9 @@ export const Modal = ({ }, [close]); useEffect(() => { - const keyDown = (e: KeyboardEvent) => { - 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); @@ -128,24 +128,26 @@ export const Modal = ({ shouldClose.current = false; }; - const handleOverlayOnMouseDown = (e: React.MouseEvent) => { - if (e.target === modalRef.current) { + const handleOverlayOnMouseDown = ( + event: React.MouseEvent + ) => { + if (event.target === modalRef.current) { shouldClose.current = true; } }; - const handleClose = (e: React.MouseEvent) => { + const handleClose = (event: React.MouseEvent) => { if (!shouldPropagateClickEvent) { - e.stopPropagation(); + event.stopPropagation(); } if (close) { close(); } }; - const handleOverlayOnClick = (e: React.MouseEvent) => { + const handleOverlayOnClick = (event: React.MouseEvent) => { if (shouldClose.current) { - handleClose(e); + handleClose(event); } }; From c8772f874266a90f3dc5a3ea74ac7a72240e42e2 Mon Sep 17 00:00:00 2001 From: vladimir-cucu Date: Mon, 25 Mar 2024 08:49:06 +0200 Subject: [PATCH 3/5] refactor: show default value of shouldPropagateClickEvent in storybook --- src/components/Modal/Modal.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Modal/Modal.tsx b/src/components/Modal/Modal.tsx index a33792241..231347b4b 100644 --- a/src/components/Modal/Modal.tsx +++ b/src/components/Modal/Modal.tsx @@ -39,7 +39,7 @@ export const Modal = ({ className, close, title, - shouldPropagateClickEvent, + shouldPropagateClickEvent = false, ...wrapperProps }: Props): ReactElement => { // list of focusable selectors is based on this Stack Overflow answer: From 9b1b103edc7d1aa31f97e52ad11105932c05f889 Mon Sep 17 00:00:00 2001 From: vladimir-cucu Date: Mon, 25 Mar 2024 09:39:52 +0200 Subject: [PATCH 4/5] test: add click event propagation tests --- .../ConfirmationModal.test.tsx | 69 +++++++++++++++++++ src/components/Modal/Modal.test.tsx | 32 +++++++++ 2 files changed, 101 insertions(+) diff --git a/src/components/ConfirmationModal/ConfirmationModal.test.tsx b/src/components/ConfirmationModal/ConfirmationModal.test.tsx index a984f374d..aca3cf9da 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).toHaveBeenCalledTimes(0); + }); + + 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).toHaveBeenCalledTimes(0); + }); + + 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/Modal/Modal.test.tsx b/src/components/Modal/Modal.test.tsx index e6a61eb25..e080c8522 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).toHaveBeenCalledTimes(0); + }); + + 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); + }); }); From 1577d3c781e0d8619482255aa5bf17be8bb1fda1 Mon Sep 17 00:00:00 2001 From: vladimir-cucu Date: Mon, 25 Mar 2024 09:55:57 +0200 Subject: [PATCH 5/5] test: use .not.toHaveBeenCalled() instead of .toHaveBeenCalledTimes(0) --- src/components/ConfirmationModal/ConfirmationModal.test.tsx | 4 ++-- src/components/Modal/Modal.test.tsx | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/ConfirmationModal/ConfirmationModal.test.tsx b/src/components/ConfirmationModal/ConfirmationModal.test.tsx index aca3cf9da..73830c53b 100644 --- a/src/components/ConfirmationModal/ConfirmationModal.test.tsx +++ b/src/components/ConfirmationModal/ConfirmationModal.test.tsx @@ -71,7 +71,7 @@ describe("ConfirmationModal ", () => { ); await userEvent.click(screen.getByText("Go back")); - expect(handleExternalClick).toHaveBeenCalledTimes(0); + expect(handleExternalClick).not.toHaveBeenCalled(); }); it("should propagate click event on cancel", async () => { @@ -104,7 +104,7 @@ describe("ConfirmationModal ", () => { ); await userEvent.click(screen.getByText("Proceed")); - expect(handleExternalClick).toHaveBeenCalledTimes(0); + expect(handleExternalClick).not.toHaveBeenCalled(); }); it("should propagate click event on confirm", async () => { diff --git a/src/components/Modal/Modal.test.tsx b/src/components/Modal/Modal.test.tsx index e080c8522..b596f1af8 100644 --- a/src/components/Modal/Modal.test.tsx +++ b/src/components/Modal/Modal.test.tsx @@ -153,7 +153,7 @@ describe("Modal ", () => { const closeButton = container.querySelector("button.p-modal__close"); expect(closeButton).not.toBeNull(); await userEvent.click(closeButton!); - expect(handleExternalClick).toHaveBeenCalledTimes(0); + expect(handleExternalClick).not.toHaveBeenCalled(); }); it("should propagate click event on close", async () => {