From 938cfcc9b7731a7fa278fe64ac4df5fd27616b0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Mon, 18 May 2020 13:37:49 +0100 Subject: [PATCH] fix(ui): only trigger useOnClickOutside when needed useOnClickOutside should start listening for events only if there's an action to take. Fixes #1754 --- .../AlertGrid/AlertGroup/Alert/AlertMenu.js | 25 ++++++------- .../AlertGroup/GroupHeader/GroupMenu.js | 13 +++---- ui/src/Components/InlineEdit/index.js | 2 +- .../Components/NavBar/FilterInput/History.js | 2 +- ui/src/Components/TooltipWrapper/index.js | 9 +++-- ui/src/Hooks/useOnClickOutside.js | 10 +++--- ui/src/Hooks/useOnClickOutside.test.js | 35 ++++++++++++++++--- 7 files changed, 62 insertions(+), 34 deletions(-) diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/AlertMenu.js b/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/AlertMenu.js index dd4b013ea..f9fc72c92 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/AlertMenu.js +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/AlertMenu.js @@ -64,9 +64,10 @@ const MenuContent = ({ ))}
{ if (Object.keys(alertStore.data.clustersWithoutReadOnly).length) { @@ -99,22 +100,22 @@ const AlertMenu = ({ setIsMenuOpen, }) => { const collapse = useLocalStore(() => ({ - value: true, + isHidden: true, toggle() { - this.value = !this.value; - setIsMenuOpen(!this.value); + this.isHidden = !this.isHidden; + setIsMenuOpen(!this.isHidden); }, hide() { - this.value = true; - setIsMenuOpen(!this.value); + this.isHidden = true; + setIsMenuOpen(!this.isHidden); }, })); - const ref = useRef(null); - useOnClickOutside(ref, collapse.hide); + const rootRef = useRef(null); + useOnClickOutside(rootRef, collapse.hide, !collapse.isHidden); return useObserver(() => ( - + {({ ref }) => ( @@ -133,7 +134,7 @@ const AlertMenu = ({ )} - + Copy link to this group
{ if (Object.keys(alertStore.data.clustersWithoutReadOnly).length) { @@ -116,11 +117,11 @@ const GroupMenu = ({ }, })); - const ref = useRef(null); - useOnClickOutside(ref, collapse.hide); + const rootRef = useRef(null); + useOnClickOutside(rootRef, collapse.hide, !collapse.isHidden); return useObserver(() => ( - + {({ ref }) => ( diff --git a/ui/src/Components/InlineEdit/index.js b/ui/src/Components/InlineEdit/index.js index 9a01745b2..24b3312a9 100644 --- a/ui/src/Components/InlineEdit/index.js +++ b/ui/src/Components/InlineEdit/index.js @@ -45,7 +45,7 @@ const InlineEdit = ({ } }; - useOnClickOutside(ref, doneEditing); + useOnClickOutside(ref, doneEditing, isEditing); useEffect(() => { if (isEditing && ref.current) { diff --git a/ui/src/Components/NavBar/FilterInput/History.js b/ui/src/Components/NavBar/FilterInput/History.js index 829ac429b..2102c3998 100644 --- a/ui/src/Components/NavBar/FilterInput/History.js +++ b/ui/src/Components/NavBar/FilterInput/History.js @@ -198,7 +198,7 @@ const History = ({ alertStore, settingsStore }) => { }); const ref = useRef(null); - useOnClickOutside(ref, collapse.hide); + useOnClickOutside(ref, collapse.hide, !collapse.isHidden); return useObserver(() => ( // data-filters is there to register filters for observation in mobx diff --git a/ui/src/Components/TooltipWrapper/index.js b/ui/src/Components/TooltipWrapper/index.js index bab8a53ac..0e4c378a9 100644 --- a/ui/src/Components/TooltipWrapper/index.js +++ b/ui/src/Components/TooltipWrapper/index.js @@ -1,4 +1,4 @@ -import React, { useState, useCallback, useEffect } from "react"; +import React, { useState, useEffect } from "react"; import { createPortal } from "react-dom"; import PropTypes from "prop-types"; @@ -26,9 +26,8 @@ const TooltipWrapper = ({ title, children, className }) => { const [isVisible, setIsVisible] = useState(false); const [wasClicked, setWasClicked] = useState(false); - const showTooltip = useCallback(() => setIsHovering(true), []); - const hideTooltip = useCallback(() => setIsHovering(false), []); - const handleClick = useCallback(() => setWasClicked(true), []); + const showTooltip = () => setIsHovering(true); + const hideTooltip = () => setIsHovering(false); useEffect(() => { let timerShow; @@ -57,7 +56,7 @@ const TooltipWrapper = ({ title, children, className }) => { return (
setWasClicked(true)} onMouseOver={supportsTouch ? null : showTooltip} onMouseLeave={supportsTouch ? null : hideTooltip} onTouchStart={supportsTouch ? showTooltip : null} diff --git a/ui/src/Hooks/useOnClickOutside.js b/ui/src/Hooks/useOnClickOutside.js index d6314da93..409eff399 100644 --- a/ui/src/Hooks/useOnClickOutside.js +++ b/ui/src/Hooks/useOnClickOutside.js @@ -1,7 +1,7 @@ import { useEffect } from "react"; // https://usehooks.com/useOnClickOutside/ -function useOnClickOutside(ref, handler) { +function useOnClickOutside(ref, handler, enabled) { useEffect(() => { const listener = (event) => { if (!ref.current || ref.current.contains(event.target)) { @@ -10,14 +10,16 @@ function useOnClickOutside(ref, handler) { handler(event); }; - document.addEventListener("mousedown", listener); - document.addEventListener("touchstart", listener); + if (enabled) { + document.addEventListener("mousedown", listener); + document.addEventListener("touchstart", listener); + } return () => { document.removeEventListener("mousedown", listener); document.removeEventListener("touchstart", listener); }; - }, [ref, handler]); + }, [ref, handler, enabled]); } export { useOnClickOutside }; diff --git a/ui/src/Hooks/useOnClickOutside.test.js b/ui/src/Hooks/useOnClickOutside.test.js index 80cd81641..269c5822f 100644 --- a/ui/src/Hooks/useOnClickOutside.test.js +++ b/ui/src/Hooks/useOnClickOutside.test.js @@ -6,10 +6,10 @@ import { mount } from "enzyme"; import { useOnClickOutside } from "./useOnClickOutside"; describe("useOnClickOutside", () => { - const Component = () => { + const Component = ({ enabled }) => { const ref = useRef(); const [isModalOpen, setModalOpen] = useState(true); - useOnClickOutside(ref, () => setModalOpen(false)); + useOnClickOutside(ref, () => setModalOpen(false), enabled); return (
@@ -25,7 +25,7 @@ describe("useOnClickOutside", () => { }; it("closes modal on click outside", () => { - const tree = mount(); + const tree = mount(); expect(tree.text()).toBe("Open"); const clickEvent = document.createEvent("MouseEvents"); @@ -38,7 +38,7 @@ describe("useOnClickOutside", () => { }); it("ignores events when hidden", () => { - const tree = mount(); + const tree = mount(); expect(tree.text()).toBe("Open"); const clickEvent = document.createEvent("MouseEvents"); @@ -54,9 +54,34 @@ describe("useOnClickOutside", () => { }); it("modal stays open on click inside", () => { - const tree = mount(); + const tree = mount(); expect(tree.text()).toBe("Open"); tree.find("span").simulate("click"); expect(tree.text()).toBe("Open"); }); + + it("only runs when enabled", () => { + const tree = mount(); + expect(tree.text()).toBe("Open"); + + const clickEvent = document.createEvent("MouseEvents"); + clickEvent.initEvent("mousedown", true, true); + act(() => { + document.dispatchEvent(clickEvent); + }); + + expect(tree.text()).toBe("Open"); + + tree.setProps({ enabled: true }); + act(() => { + document.dispatchEvent(clickEvent); + }); + expect(tree.text()).toBe("Hidden"); + }); + + it("unmounts cleanly", () => { + const tree = mount(); + expect(tree.text()).toBe("Open"); + tree.unmount(); + }); });