From 23eb2c6f54b4f41788b4dff2f76fcedb4ace326f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Sun, 31 May 2020 16:15:28 +0100 Subject: [PATCH] fix(ui): avoid annotation re-renders afterUpdate callback is chaning on every render, triggering annotation re-render --- .../Grid/AlertGrid/AlertGroup/Alert/index.js | 1 - .../AlertGrid/AlertGroup/Annotation/index.js | 4 +-- .../AlertGroup/Annotation/index.test.js | 10 ------ .../AlertGrid/AlertGroup/GroupFooter/index.js | 1 - .../Grid/AlertGrid/AlertGroup/index.js | 14 ++++---- .../Grid/AlertGrid/AlertGroup/index.test.js | 1 + ui/src/Components/Grid/AlertGrid/Grid.js | 36 +++++++++---------- .../Components/Grid/AlertGrid/index.test.js | 12 +++---- ui/src/Hooks/useGrid.js | 16 ++++----- ui/src/Hooks/useGrid.test.js | 9 ++++- 10 files changed, 47 insertions(+), 57 deletions(-) diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.js b/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.js index 911920c85..9e5677a00 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.js +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.js @@ -69,7 +69,6 @@ const Alert = ({ value={a.value} visible={a.visible} afterUpdate={afterUpdate} - alertStore={alertStore} /> ))} diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/Annotation/index.js b/ui/src/Components/Grid/AlertGrid/AlertGroup/Annotation/index.js index 7802b786a..fbf0b57d2 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/Annotation/index.js +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/Annotation/index.js @@ -12,11 +12,10 @@ import { faExternalLinkAlt } from "@fortawesome/free-solid-svg-icons/faExternalL import { faSearchPlus } from "@fortawesome/free-solid-svg-icons/faSearchPlus"; import { faSearchMinus } from "@fortawesome/free-solid-svg-icons/faSearchMinus"; -import { AlertStore } from "Stores/AlertStore"; import { TooltipWrapper } from "Components/TooltipWrapper"; const RenderNonLinkAnnotation = observer( - ({ alertStore, name, value, visible, afterUpdate }) => { + ({ name, value, visible, afterUpdate }) => { const mountRef = useRef(false); useEffect(() => { @@ -74,7 +73,6 @@ const RenderNonLinkAnnotation = observer( } ); RenderNonLinkAnnotation.propTypes = { - alertStore: PropTypes.instanceOf(AlertStore).isRequired, name: PropTypes.string.isRequired, value: PropTypes.string.isRequired, visible: PropTypes.bool.isRequired, diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/Annotation/index.test.js b/ui/src/Components/Grid/AlertGrid/AlertGroup/Annotation/index.test.js index 3a553d0b9..8bd89d8fd 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/Annotation/index.test.js +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/Annotation/index.test.js @@ -4,15 +4,8 @@ import { shallow, mount } from "enzyme"; import toDiffableHtml from "diffable-html"; -import { AlertStore } from "Stores/AlertStore"; import { RenderNonLinkAnnotation, RenderLinkAnnotation } from "."; -let alertStore; - -beforeEach(() => { - alertStore = new AlertStore([]); -}); - const ShallowLinkAnnotation = () => { return shallow( @@ -38,7 +31,6 @@ const MockAfterUpdate = jest.fn(); const ShallowNonLinkAnnotation = (visible) => { return shallow( { const MountedNonLinkAnnotation = (visible) => { return mount( { const MountedNonLinkAnnotationContainingLink = (visible) => { return mount( ))} diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/index.js b/ui/src/Components/Grid/AlertGrid/AlertGroup/index.js index be577ec8b..cbb591c5a 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/index.js +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/index.js @@ -55,7 +55,7 @@ const AlertGroup = observer( alertStore: PropTypes.instanceOf(AlertStore).isRequired, settingsStore: PropTypes.instanceOf(Settings).isRequired, silenceFormStore: PropTypes.instanceOf(SilenceFormStore).isRequired, - style: PropTypes.object, + groupWidth: PropTypes.number.isRequired, gridLabelValue: PropTypes.string.isRequired, }; @@ -185,7 +185,7 @@ const AlertGroup = observer( silenceFormStore, alertStore, settingsStore, - style, + groupWidth, gridLabelValue, } = this.props; @@ -221,11 +221,6 @@ const AlertGroup = observer( } } - let extraStyle = {}; - if (this.renderConfig.isMenuOpen) { - extraStyle.zIndex = 100; - } - return (
{ alertStore={alertStore} silenceFormStore={silenceFormStore} gridLabelValue="" + groupWidth={420} />, { wrappingComponent: ThemeContext.Provider, diff --git a/ui/src/Components/Grid/AlertGrid/Grid.js b/ui/src/Components/Grid/AlertGrid/Grid.js index 44bbdc0ea..15db7c0bb 100644 --- a/ui/src/Components/Grid/AlertGrid/Grid.js +++ b/ui/src/Components/Grid/AlertGrid/Grid.js @@ -32,7 +32,7 @@ const Grid = ({ outerPadding, }) => { const { ref, repack } = useGrid(gridSizesConfig); - const debouncedRepack = debounce(repack, 10); + const debouncedRepack = useCallback(debounce(repack, 10), [repack]); const [groupsToRender, setGroupsToRender] = useState(50); @@ -115,23 +115,23 @@ const Grid = ({ }} > {isExpanded || grid.labelName === "" - ? grid.alertGroups.slice(0, groupsToRender).map((group) => ( - 1 - } - afterUpdate={debouncedRepack} - alertStore={alertStore} - settingsStore={settingsStore} - silenceFormStore={silenceFormStore} - style={{ - width: groupWidth, - }} - gridLabelValue={grid.labelValue} - /> - )) + ? grid.alertGroups + .slice(0, groupsToRender) + .map((group) => ( + 1 + } + afterUpdate={debouncedRepack} + alertStore={alertStore} + settingsStore={settingsStore} + silenceFormStore={silenceFormStore} + groupWidth={groupWidth} + gridLabelValue={grid.labelValue} + /> + )) : []}
{isExpanded && grid.alertGroups.length > groupsToRender && ( diff --git a/ui/src/Components/Grid/AlertGrid/index.test.js b/ui/src/Components/Grid/AlertGrid/index.test.js index 585bfa02c..df226bf63 100644 --- a/ui/src/Components/Grid/AlertGrid/index.test.js +++ b/ui/src/Components/Grid/AlertGrid/index.test.js @@ -337,7 +337,7 @@ describe("", () => { groupWidth: wrapper.instance().viewport.groupWidth, }); - expect(tree.find("AlertGroup").at(0).props().style.width).toBe( + expect(tree.find("AlertGroup").at(0).props().groupWidth).toBe( Math.floor(innerWidth / columns) ); }; @@ -419,7 +419,7 @@ describe("", () => { gridSizesConfig: wrapper.instance().viewport.gridSizesConfig, groupWidth: wrapper.instance().viewport.groupWidth, }); - expect(tree.find("AlertGroup").at(0).props().style.width).toBe(1980 / 4); + expect(tree.find("AlertGroup").at(0).props().groupWidth).toBe(1980 / 4); // then resize and verify if column count was changed wrapper.instance().viewport.updateWidths(1000, 1000); @@ -427,7 +427,7 @@ describe("", () => { gridSizesConfig: wrapper.instance().viewport.gridSizesConfig, groupWidth: wrapper.instance().viewport.groupWidth, }); - expect(tree.find("AlertGroup").at(0).props().style.width).toBe(1000 / 2); + expect(tree.find("AlertGroup").at(0).props().groupWidth).toBe(1000 / 2); }); it("scrollbar render doesn't resize alert groups", () => { @@ -443,7 +443,7 @@ describe("", () => { gridSizesConfig: wrapper.instance().viewport.gridSizesConfig, groupWidth: wrapper.instance().viewport.groupWidth, }); - expect(tree.find("AlertGroup").at(0).props().style.width).toBe(400); + expect(tree.find("AlertGroup").at(0).props().groupWidth).toBe(400); // then resize and verify if column count was changed wrapper.instance().viewport.updateWidths(1584, 1600); @@ -451,7 +451,7 @@ describe("", () => { gridSizesConfig: wrapper.instance().viewport.gridSizesConfig, groupWidth: wrapper.instance().viewport.groupWidth, }); - expect(tree.find("AlertGroup").at(0).props().style.width).toBe(396); + expect(tree.find("AlertGroup").at(0).props().groupWidth).toBe(396); }); it("window resize event calls updateWidths", () => { @@ -480,7 +480,7 @@ describe("", () => { gridSizesConfig: wrapper.instance().viewport.gridSizesConfig, groupWidth: wrapper.instance().viewport.groupWidth, }); - results.push(tree.find("AlertGroup").at(0).props().style.width); + results.push(tree.find("AlertGroup").at(0).props().groupWidth); } expect(results).toStrictEqual([ diff --git a/ui/src/Hooks/useGrid.js b/ui/src/Hooks/useGrid.js index 53fa38d3d..380441ac9 100644 --- a/ui/src/Hooks/useGrid.js +++ b/ui/src/Hooks/useGrid.js @@ -1,16 +1,11 @@ -import { useEffect, useRef } from "react"; +import { useEffect, useRef, useState } from "react"; import Bricks from "bricks.js"; const useGrid = (sizes) => { const ref = useRef(null); const grid = useRef(null); - - const repack = () => { - if (grid.current) { - grid.current.pack(); - } - }; + const [repack, setRepack] = useState(() => () => {}); useEffect(() => { if (!grid.current && ref.current) { @@ -20,12 +15,15 @@ const useGrid = (sizes) => { packed: "packed", position: false, }); - window.addEventListener("resize", repack); + window.addEventListener("resize", grid.current.pack); grid.current.pack(); + setRepack(() => () => { + grid.current && grid.current.pack(); + }); } return () => { - window.removeEventListener("resize", repack); + if (grid.current) window.removeEventListener("resize", grid.current.pack); grid.current = null; }; }, [sizes]); diff --git a/ui/src/Hooks/useGrid.test.js b/ui/src/Hooks/useGrid.test.js index ca5d07d1c..cfd318949 100644 --- a/ui/src/Hooks/useGrid.test.js +++ b/ui/src/Hooks/useGrid.test.js @@ -1,6 +1,6 @@ import React from "react"; -import { renderHook, act } from "@testing-library/react-hooks"; +import { renderHook } from "@testing-library/react-hooks"; import { mount } from "enzyme"; @@ -57,4 +57,11 @@ describe("useGrid", () => { const tree = mount(); tree.unmount(); }); + + it("repack after unmount does nothing", () => { + const tree = mount(); + const repack = tree.find("#root").props().onClick; + tree.unmount(); + repack(); + }); });