Skip to content

Commit

Permalink
[Enhancement] avoid calling mapPopover setstate infinitely (#1346)
Browse files Browse the repository at this point in the history 
* make mappopover a functional component add useLayoutEffect hook
* add map-popover test

Signed-off-by: Shan He <heshan0131@gmail.com>
  • Loading branch information
@heshan0131
heshan0131 committed Dec 10, 2020
1 parent ae234e7 commit a11c63c
Show file tree
Hide file tree
Showing 5 changed files with 235 additions and 118 deletions.
1 change: 1 addition & 0 deletions package.json
View file
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion src/components/map/coordinate-info.js
View file
Expand Up @@ -30,7 +30,7 @@ const DECIMAL_Z = 1;

const CoordinateInfoFactory = () => {
const CoordinateInfo = ({coordinate, zoom}) => (
<div>
<div className="coordingate-hover-info">
<StyledLayerName className="map-popover__layer-name">
<CursorClick height="12px" />
Coordinate
Expand Down
274 changes: 163 additions & 111 deletions src/components/map/map-popover.js
View file
Expand Up @@ -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';
Expand Down Expand Up @@ -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 (
<ErrorBoundary>
<StyledMapPopover
ref={this.popover}
className="map-popover"
style={{
...style,
maxWidth: MAX_WIDTH
}}
>
{frozen ? (
<div className="map-popover__top">
<div className="gutter" />
{!isLeft && (
<StyledIcon className="popover-arrow-left" onClick={this.moveLeft}>
<ArrowLeft />
</StyledIcon>
)}
<StyledIcon className="popover-pin" onClick={this.props.onClose}>
<Pin height="16px" />
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 (
<ErrorBoundary>
<StyledMapPopover
ref={popover}
className="map-popover"
style={{
...pos,
maxWidth: MAX_WIDTH,
maxHeight: MAX_HEIGHT
}}
>
{frozen ? (
<div className="map-popover__top">
<div className="gutter" />
{!isLeft && (
<StyledIcon className="popover-arrow-left" onClick={moveLeft}>
<ArrowLeft />
</StyledIcon>
{isLeft && (
<StyledIcon className="popover-arrow-right" onClick={this.moveRight}>
<ArrowRight />
</StyledIcon>
)}
{isBase && (
<div className="primary-label">
<FormattedMessage id="mapPopover.primary" />
</div>
)}
</div>
) : null}
{Array.isArray(coordinate) && <CoordinateInfo coordinate={coordinate} zoom={zoom} />}
{layerHoverProp && <LayerHoverInfo {...layerHoverProp} />}
</StyledMapPopover>
</ErrorBoundary>
);
}
}
)}
<StyledIcon className="popover-pin" onClick={onClose}>
<Pin height="16px" />
</StyledIcon>
{isLeft && (
<StyledIcon className="popover-arrow-right" onClick={moveRight}>
<ArrowRight />
</StyledIcon>
)}
{isBase && (
<div className="primary-label">
<FormattedMessage id="mapPopover.primary" />
</div>
)}
</div>
) : null}
{Array.isArray(coordinate) && <CoordinateInfo coordinate={coordinate} zoom={zoom} />}
{layerHoverProp && <LayerHoverInfo {...layerHoverProp} />}
</StyledMapPopover>
</ErrorBoundary>
);
};

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);
}
52 changes: 47 additions & 5 deletions test/browser/components/map/map-popover-test.js
View file
Expand Up @@ -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 = () => <div className="layer-hover-info" />;
const CoordinateInfo = () => <div className="coordingate-hover-info" />;
const MapPopover = MapPopoverFactory(LayerHoverInfo, CoordinateInfo);
let wrapper;
const onClose = sinon.spy();
t.doesNotThrow(() => {
Expand All @@ -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}) => <IntlWrapper>{children}</IntlWrapper>;
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();
});

0 comments on commit a11c63c

Please sign in to comment.