From a11c63c32a6df87d5da7b416e074dc2276e570b1 Mon Sep 17 00:00:00 2001 From: Shan He Date: Thu, 10 Dec 2020 09:00:05 -0800 Subject: [PATCH] [Enhancement] avoid calling mapPopover setstate infinitely (#1346) * make mappopover a functional component add useLayoutEffect hook * add map-popover test Signed-off-by: Shan He --- package.json | 1 + src/components/map/coordinate-info.js | 2 +- src/components/map/map-popover.js | 274 +++++++++++------- .../components/map/map-popover-test.js | 52 +++- yarn.lock | 24 +- 5 files changed, 235 insertions(+), 118 deletions(-) diff --git a/package.json b/package.json index cecc0e0816..45f2b5994c 100644 --- a/package.json +++ b/package.json @@ -179,6 +179,7 @@ "@deck.gl/test-utils": "^8.2.0", "@luma.gl/test-utils": "^8.2.0", "@probe.gl/test-utils": "^3.0.1", + "@testing-library/react-hooks": "^3.4.2", "@types/d3-array": "^2.0.0", "babel-eslint": "^10.1.0", "babel-loader": "^8.0.0", diff --git a/src/components/map/coordinate-info.js b/src/components/map/coordinate-info.js index bf6af4e103..f3c8050924 100644 --- a/src/components/map/coordinate-info.js +++ b/src/components/map/coordinate-info.js @@ -30,7 +30,7 @@ const DECIMAL_Z = 1; const CoordinateInfoFactory = () => { const CoordinateInfo = ({coordinate, zoom}) => ( -
+
Coordinate diff --git a/src/components/map/map-popover.js b/src/components/map/map-popover.js index 19ebd61e23..ee4f770ce4 100644 --- a/src/components/map/map-popover.js +++ b/src/components/map/map-popover.js @@ -18,7 +18,7 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. -import React, {PureComponent, createRef} from 'react'; +import React, {useState, useCallback, useRef, useLayoutEffect} from 'react'; import styled from 'styled-components'; import PropTypes from 'prop-types'; import LayerHoverInfoFactory from './layer-hover-info'; @@ -107,129 +107,181 @@ const StyledIcon = styled.div` MapPopoverFactory.deps = [LayerHoverInfoFactory, CoordinateInfoFactory]; -export default function MapPopoverFactory(LayerHoverInfo, CoordinateInfo) { - class MapPopover extends PureComponent { - static propTypes = { - layerHoverProp: PropTypes.object, - coordinate: PropTypes.oneOfType([PropTypes.array, PropTypes.bool]), - frozen: PropTypes.bool, - x: PropTypes.number, - y: PropTypes.number, - z: PropTypes.number, - mapW: PropTypes.number.isRequired, - mapH: PropTypes.number.isRequired, - onClose: PropTypes.func.isRequired, - isBase: PropTypes.bool - }; - - constructor(props) { - super(props); - this.state = { - width: 380, - height: 160, - isLeft: false - }; - } +export function getPosition({x, y, mapW, mapH, width, height, isLeft}) { + const topOffset = 20; + const leftOffset = 20; + if (![x, y, mapW, mapH, width, height].every(Number.isFinite)) { + return {}; + } - componentDidMount() { - this._setContainerSize(); - } + const pos = {}; + if (x + leftOffset + width > mapW || isLeft) { + pos.right = mapW - x + leftOffset; + } else { + pos.left = x + leftOffset; + } + + if (y + topOffset + height > mapH) { + pos.bottom = 10; + } else { + pos.top = y + topOffset; + } + + return pos; +} - componentDidUpdate() { - this._setContainerSize(); +function hasPosChanged({oldPos = {}, newPos = {}}) { + for (const key in newPos) { + if (oldPos[key] !== newPos[key]) { + return true; } + } + for (const key in oldPos) { + if (oldPos[key] !== newPos[key]) { + return true; + } + } + return false; +} - popover = createRef(); +// hooks +export function usePosition({layerHoverProp, x, y, mapW, mapH}, popover) { + const [isLeft, setIsLeft] = useState(false); + const [pos, setPosition] = useState({}); - _setContainerSize() { - const node = this.popover.current; - if (!node) { - return; - } + const moveLeft = useCallback(() => setIsLeft(true), [setIsLeft]); + const moveRight = useCallback(() => setIsLeft(false), [setIsLeft]); + const hoverData = layerHoverProp && layerHoverProp.data; - const width = Math.min(Math.round(node.scrollWidth), MAX_WIDTH); - const height = Math.min(Math.round(node.scrollHeight), MAX_HEIGHT); + useLayoutEffect(() => { + const node = popover.current; - if (width !== this.state.width || height !== this.state.height) { - this.setState({width, height}); - } + if (!node || !hoverData) { + return; } - _getPosition(x, y, isLeft) { - const topOffset = 20; - const leftOffset = 20; - const {mapW, mapH} = this.props; - const {width, height} = this.state; - const pos = {}; - if (x + leftOffset + width > mapW || isLeft) { - pos.right = mapW - x + leftOffset; - } else { - pos.left = x + leftOffset; + const width = Math.round(node.offsetWidth); + const height = Math.round(node.offsetHeight); + + if (Number.isFinite(width) && width > 0 && Number.isFinite(height) && height > 0) { + const newPos = getPosition({ + x, + y, + mapW, + mapH, + width, + height, + isLeft + }); + if (hasPosChanged({oldPos: pos, newPos})) { + setPosition(newPos); } - - if (y + topOffset + height > mapH) { - pos.bottom = 10; - } else { - pos.top = y + topOffset; - } - - return pos; } + }, [x, y, mapH, mapW, isLeft, hoverData, pos, popover]); - moveLeft = () => { - this.setState({isLeft: true}); - }; - - moveRight = () => { - this.setState({isLeft: false}); - }; - - render() { - const {x, y, frozen, coordinate, layerHoverProp, isBase, zoom} = this.props; - const {isLeft} = this.state; - - const style = Number.isFinite(x) && Number.isFinite(y) ? this._getPosition(x, y, isLeft) : {}; - - return ( - - - {frozen ? ( -
-
- {!isLeft && ( - - - - )} - - + return {moveLeft, moveRight, isLeft, pos}; +} +export default function MapPopoverFactory(LayerHoverInfo, CoordinateInfo) { + const MapPopover = ({ + x, + y, + mapW, + mapH, + frozen, + coordinate, + layerHoverProp, + isBase, + zoom, + onClose + }) => { + const popover = useRef(null); + const {moveLeft, moveRight, isLeft, pos} = usePosition( + {layerHoverProp, x, y, mapW, mapH}, + popover + ); + // const [isLeft, setIsLeft] = useState(false); + // const [pos, setPosition] = useState({}); + + // const moveLeft = useCallback(() => setIsLeft(true), [setIsLeft]); + // const moveRight = useCallback(() => setIsLeft(false), [setIsLeft]); + // const hoverData = layerHoverProp && layerHoverProp.data; + + // useLayoutEffect(() => { + // const node = popover.current; + // if (!node || !hoverData) { + // return; + // } + // const width = Math.round(node.offsetWidth); + // const height = Math.round(node.offsetHeight); + + // if (Number.isFinite(width) && width > 0 && Number.isFinite(height) && height > 0) { + // setPosition( + // getPosition({ + // x, + // y, + // mapW, + // mapH, + // width, + // height, + // isLeft + // }) + // ); + // } + // }, [x, y, mapH, mapW, isLeft, hoverData]); + + return ( + + + {frozen ? ( +
+
+ {!isLeft && ( + + - {isLeft && ( - - - - )} - {isBase && ( -
- -
- )} -
- ) : null} - {Array.isArray(coordinate) && } - {layerHoverProp && } - - - ); - } - } + )} + + + + {isLeft && ( + + + + )} + {isBase && ( +
+ +
+ )} +
+ ) : null} + {Array.isArray(coordinate) && } + {layerHoverProp && } + + + ); + }; + + MapPopover.propTypes = { + layerHoverProp: PropTypes.object, + coordinate: PropTypes.oneOfType([PropTypes.array, PropTypes.bool]), + frozen: PropTypes.bool, + x: PropTypes.number, + y: PropTypes.number, + z: PropTypes.number, + mapW: PropTypes.number.isRequired, + mapH: PropTypes.number.isRequired, + onClose: PropTypes.func.isRequired, + isBase: PropTypes.bool + }; return injectIntl(MapPopover); } diff --git a/test/browser/components/map/map-popover-test.js b/test/browser/components/map/map-popover-test.js index 8e0ce6e648..7861132b2e 100644 --- a/test/browser/components/map/map-popover-test.js +++ b/test/browser/components/map/map-popover-test.js @@ -21,13 +21,16 @@ import React from 'react'; import sinon from 'sinon'; import test from 'tape'; +import {renderHook} from '@testing-library/react-hooks'; + import {Pin} from 'components/common/icons'; import {IntlWrapper, mountWithTheme} from 'test/helpers/component-utils'; -import MapPopoverFactory from 'components/map/map-popover'; +import MapPopoverFactory, {usePosition} from 'components/map/map-popover'; +import {appInjector} from 'components'; + +const MapPopover = appInjector.get(MapPopoverFactory); + test('Map Popover - render', t => { - const LayerHoverInfo = () =>
; - const CoordinateInfo = () =>
; - const MapPopover = MapPopoverFactory(LayerHoverInfo, CoordinateInfo); let wrapper; const onClose = sinon.spy(); t.doesNotThrow(() => { @@ -48,9 +51,48 @@ test('Map Popover - render', t => { t.equal(wrapper.find(MapPopover).length, 1, 'Should display 1 MapPopover'); t.equal(wrapper.find('.coordingate-hover-info').length, 1, 'Should display 1 coordinates'); - t.equal(wrapper.find('.layer-hover-info').length, 1, 'Should display 1 layer info'); + t.equal(wrapper.find('.map-popover__layer-info').length, 0, 'Should display 0 layer info'); t.equal(wrapper.find(Pin).length, 1, 'Should display 1 pin'); t.equal(wrapper.find('.primary-label').length, 1, 'Should display 1 primary label'); t.end(); }); + +test('Map Popover -> renderHooks', t => { + const wrapper = ({children}) => {children}; + const ref = { + current: { + offsetWidth: 50, + offsetHeight: 50 + } + }; + const layerHoverProp = {data: [1]}; + + const {result} = renderHook( + () => + usePosition( + { + x: 100, + y: 200, + mapW: 600, + mapH: 800, + layerHoverProp + }, + ref + ), + { + wrapper + } + ); + + t.deepEqual( + result.current.pos, + { + left: 120, + top: 220 + }, + 'should calculate correct pos' + ); + + t.end(); +}); diff --git a/yarn.lock b/yarn.lock index b4bf71a9bf..254e784118 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1074,7 +1074,7 @@ pirates "^4.0.0" source-map-support "^0.5.16" -"@babel/runtime@^7.0.0", "@babel/runtime@^7.12.1", "@babel/runtime@^7.2.0", "@babel/runtime@^7.3.1", "@babel/runtime@^7.7.2", "@babel/runtime@^7.8.4", "@babel/runtime@^7.8.7": +"@babel/runtime@^7.0.0", "@babel/runtime@^7.12.1", "@babel/runtime@^7.12.5", "@babel/runtime@^7.2.0", "@babel/runtime@^7.3.1", "@babel/runtime@^7.7.2", "@babel/runtime@^7.8.4", "@babel/runtime@^7.8.7": version "7.12.5" resolved "https://registry.yarnpkg.com/@babel/runtime/-/runtime-7.12.5.tgz#410e7e487441e1b360c29be715d870d9b985882e" integrity sha512-plcc+hbExy3McchJCEQG3knOsuh3HH+Prx1P6cLIkET/0dLuQDEnrT+s27Axgc9bqfsmNUNHfscgMUdBpC9xfg== @@ -1987,6 +1987,14 @@ remark "^13.0.0" unist-util-find-all-after "^3.0.2" +"@testing-library/react-hooks@^3.4.2": + version "3.7.0" + resolved "https://registry.yarnpkg.com/@testing-library/react-hooks/-/react-hooks-3.7.0.tgz#6d75c5255ef49bce39b6465bf6b49e2dac84919e" + integrity sha512-TwfbY6BWtWIHitjT05sbllyLIProcysC0dF0q1bbDa7OHLC6A6rJOYJwZ13hzfz3O4RtOuInmprBozJRyyo7/g== + dependencies: + "@babel/runtime" "^7.12.5" + "@types/testing-library__react-hooks" "^3.4.0" + "@turf/bbox@6.x", "@turf/bbox@^6.0.1": version "6.0.1" resolved "https://registry.yarnpkg.com/@turf/bbox/-/bbox-6.0.1.tgz#b966075771475940ee1c16be2a12cf389e6e923a" @@ -2188,6 +2196,13 @@ resolved "https://registry.yarnpkg.com/@types/prop-types/-/prop-types-15.7.3.tgz#2ab0d5da2e5815f94b0b9d4b95d1e5f243ab2ca7" integrity sha512-KfRL3PuHmqQLOG+2tGpRO26Ctg+Cq1E01D2DMriKEATHgWLfeNDmq9e29Q9WIky0dQ3NPkd1mzYH8Lm936Z9qw== +"@types/react-test-renderer@*": + version "17.0.0" + resolved "https://registry.yarnpkg.com/@types/react-test-renderer/-/react-test-renderer-17.0.0.tgz#9be47b375eeb906fced37049e67284a438d56620" + integrity sha512-nvw+F81OmyzpyIE1S0xWpLonLUZCMewslPuA8BtjSKc5XEbn8zEQBXS7KuOLHTNnSOEM2Pum50gHOoZ62tqTRg== + dependencies: + "@types/react" "*" + "@types/react@*": version "17.0.0" resolved "https://registry.yarnpkg.com/@types/react/-/react-17.0.0.tgz#5af3eb7fad2807092f0046a1302b7823e27919b8" @@ -2211,6 +2226,13 @@ resolved "https://registry.yarnpkg.com/@types/tapable/-/tapable-1.0.6.tgz#a9ca4b70a18b270ccb2bc0aaafefd1d486b7ea74" integrity sha512-W+bw9ds02rAQaMvaLYxAbJ6cvguW/iJXNT6lTssS1ps6QdrMKttqEAMEG/b5CR8TZl3/L7/lH0ZV5nNR1LXikA== +"@types/testing-library__react-hooks@^3.4.0": + version "3.4.1" + resolved "https://registry.yarnpkg.com/@types/testing-library__react-hooks/-/testing-library__react-hooks-3.4.1.tgz#b8d7311c6c1f7db3103e94095fe901f8fef6e433" + integrity sha512-G4JdzEcq61fUyV6wVW9ebHWEiLK2iQvaBuCHHn9eMSbZzVh4Z4wHnUGIvQOYCCYeu5DnUtFyNYuAAgbSaO/43Q== + dependencies: + "@types/react-test-renderer" "*" + "@types/uglify-js@*": version "3.11.1" resolved "https://registry.yarnpkg.com/@types/uglify-js/-/uglify-js-3.11.1.tgz#97ff30e61a0aa6876c270b5f538737e2d6ab8ceb"