justinpark commented on code in PR #26736:
URL: https://github.com/apache/superset/pull/26736#discussion_r1466762113
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.jsx:
##########
@@ -69,87 +95,82 @@ class MapBox extends React.Component {
}).fitBounds(bounds);
const { latitude, longitude, zoom } = mercator;
- this.state = {
- viewport: {
+ if (initialViewportSettings === 'Fixed') {
+ setViewport({
+ longitude: !isEmpty(viewportLongitude) ? viewportLongitude : longitude,
+ latitude: !isEmpty(viewportLatitude) ? viewportLatitude : latitude,
+ zoom: !isEmpty(viewportZoom) ? viewportZoom : zoom,
+ });
+ } else {
+ setViewport({
longitude,
latitude,
zoom,
- },
- };
- this.handleViewportChange = this.handleViewportChange.bind(this);
- }
+ });
+ }
+ }, []);
- handleViewportChange(viewport) {
- this.setState({ viewport });
- const { onViewportChange } = this.props;
- onViewportChange(viewport);
+ useEffect(() => {
+ if (viewport) {
+ // Compute the clusters based on the original bounds and current zoom
level. Note when zoom/pan
+ // to an area outside of the original bounds, no additional queries are
made to the backend to
+ // retrieve additional data.
+ // add this variable to widen the visible area
+ const offsetHorizontal = (width * 0.5) / 100;
+ const offsetVertical = (height * 0.5) / 100;
+ const bbox = [
+ bounds[0][0] - offsetHorizontal,
+ bounds[0][1] - offsetVertical,
+ bounds[1][0] + offsetHorizontal,
+ bounds[1][1] + offsetVertical,
+ ];
+ setClusters(clusterer.getClusters(bbox, Math.round(viewport.zoom)));
+ }
+ }, [clusterer, viewport]);
Review Comment:
Given that `clusters` is assigned solely based on the `viewport` value,
utilizing `useMemo` would be a more efficient approach compared to a
combination of `useState` and `useEffect`.
```suggestion
useEffect(() => {
if (viewport) {
// Compute the clusters based on the original bounds and current zoom
level. Note when zoom/pan
// to an area outside of the original bounds, no additional queries
are made to the backend to
// retrieve additional data.
// add this variable to widen the visible area
const offsetHorizontal = (width * 0.5) / 100;
const offsetVertical = (height * 0.5) / 100;
const bbox = [
bounds[0][0] - offsetHorizontal,
bounds[0][1] - offsetVertical,
bounds[1][0] + offsetHorizontal,
bounds[1][1] + offsetVertical,
];
setClusters(clusterer.getClusters(bbox, Math.round(viewport.zoom)));
}
}, [clusters, viewport]);
const clusters = useMemo(() => {
if (viewport) {
// Compute the clusters based on the original bounds and current zoom
level. Note when zoom/pan
// to an area outside of the original bounds, no additional queries
are made to the backend to
// retrieve additional data.
// add this variable to widen the visible area
const offsetHorizontal = (width * 0.5) / 100;
const offsetVertical = (height * 0.5) / 100;
const bbox = [
bounds[0][0] - offsetHorizontal,
bounds[0][1] - offsetVertical,
bounds[1][0] + offsetHorizontal,
bounds[1][1] + offsetVertical,
];
return clusterer.getClusters(bbox, Math.round(viewport.zoom));
}
return undefined;
}, [viewport]);
```
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.jsx:
##########
@@ -44,6 +44,10 @@ const propTypes = {
renderWhileDragging: PropTypes.bool,
rgb: PropTypes.array,
bounds: PropTypes.array,
+ viewportLongitude: PropTypes.number,
+ viewportLatitude: PropTypes.number,
+ viewportZoom: PropTypes.number,
+ initialViewportSettings: PropTypes.string,
Review Comment:
```suggestion
initialViewportSettings: PropTypes.oneOf(['Auto', 'Fixed']),
```
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.jsx:
##########
@@ -69,87 +95,82 @@ class MapBox extends React.Component {
}).fitBounds(bounds);
const { latitude, longitude, zoom } = mercator;
- this.state = {
- viewport: {
+ if (initialViewportSettings === 'Fixed') {
+ setViewport({
+ longitude: !isEmpty(viewportLongitude) ? viewportLongitude : longitude,
+ latitude: !isEmpty(viewportLatitude) ? viewportLatitude : latitude,
+ zoom: !isEmpty(viewportZoom) ? viewportZoom : zoom,
+ });
+ } else {
+ setViewport({
longitude,
latitude,
zoom,
- },
- };
- this.handleViewportChange = this.handleViewportChange.bind(this);
- }
+ });
+ }
+ }, []);
- handleViewportChange(viewport) {
- this.setState({ viewport });
- const { onViewportChange } = this.props;
- onViewportChange(viewport);
+ useEffect(() => {
+ if (viewport) {
+ // Compute the clusters based on the original bounds and current zoom
level. Note when zoom/pan
+ // to an area outside of the original bounds, no additional queries are
made to the backend to
+ // retrieve additional data.
+ // add this variable to widen the visible area
+ const offsetHorizontal = (width * 0.5) / 100;
+ const offsetVertical = (height * 0.5) / 100;
+ const bbox = [
+ bounds[0][0] - offsetHorizontal,
+ bounds[0][1] - offsetVertical,
+ bounds[1][0] + offsetHorizontal,
+ bounds[1][1] + offsetVertical,
+ ];
+ setClusters(clusterer.getClusters(bbox, Math.round(viewport.zoom)));
+ }
+ }, [clusterer, viewport]);
+
+ if (!viewport || !clusters) {
+ return <></>;
}
- render() {
- const {
- width,
- height,
- aggregatorName,
- clusterer,
- globalOpacity,
- mapStyle,
- mapboxApiKey,
- pointRadius,
- pointRadiusUnit,
- renderWhileDragging,
- rgb,
- hasCustomMetric,
- bounds,
- } = this.props;
- const { viewport } = this.state;
- const isDragging =
- viewport.isDragging === undefined ? false : viewport.isDragging;
-
- // Compute the clusters based on the original bounds and current zoom
level. Note when zoom/pan
- // to an area outside of the original bounds, no additional queries are
made to the backend to
- // retrieve additional data.
- // add this variable to widen the visible area
- const offsetHorizontal = (width * 0.5) / 100;
- const offsetVertical = (height * 0.5) / 100;
- const bbox = [
- bounds[0][0] - offsetHorizontal,
- bounds[0][1] - offsetVertical,
- bounds[1][0] + offsetHorizontal,
- bounds[1][1] + offsetVertical,
- ];
- const clusters = clusterer.getClusters(bbox, Math.round(viewport.zoom));
-
- return (
- <MapGL
+ const isDragging =
+ viewport.isDragging === undefined ? false : viewport.isDragging;
+
+ const handleViewportChange = newViewport => {
+ setViewport(newViewport);
+ const { onViewportChange } = props;
Review Comment:
It would be more efficient to consolidate all prop spreading in a single
location, such as line 83.
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.jsx:
##########
@@ -69,87 +95,82 @@ class MapBox extends React.Component {
}).fitBounds(bounds);
const { latitude, longitude, zoom } = mercator;
- this.state = {
- viewport: {
+ if (initialViewportSettings === 'Fixed') {
+ setViewport({
+ longitude: !isEmpty(viewportLongitude) ? viewportLongitude : longitude,
+ latitude: !isEmpty(viewportLatitude) ? viewportLatitude : latitude,
+ zoom: !isEmpty(viewportZoom) ? viewportZoom : zoom,
+ });
+ } else {
+ setViewport({
longitude,
latitude,
zoom,
- },
- };
- this.handleViewportChange = this.handleViewportChange.bind(this);
- }
+ });
+ }
+ }, []);
- handleViewportChange(viewport) {
- this.setState({ viewport });
- const { onViewportChange } = this.props;
- onViewportChange(viewport);
+ useEffect(() => {
+ if (viewport) {
+ // Compute the clusters based on the original bounds and current zoom
level. Note when zoom/pan
+ // to an area outside of the original bounds, no additional queries are
made to the backend to
+ // retrieve additional data.
+ // add this variable to widen the visible area
+ const offsetHorizontal = (width * 0.5) / 100;
+ const offsetVertical = (height * 0.5) / 100;
+ const bbox = [
+ bounds[0][0] - offsetHorizontal,
+ bounds[0][1] - offsetVertical,
+ bounds[1][0] + offsetHorizontal,
+ bounds[1][1] + offsetVertical,
+ ];
+ setClusters(clusterer.getClusters(bbox, Math.round(viewport.zoom)));
+ }
+ }, [clusterer, viewport]);
+
+ if (!viewport || !clusters) {
+ return <></>;
}
- render() {
- const {
- width,
- height,
- aggregatorName,
- clusterer,
- globalOpacity,
- mapStyle,
- mapboxApiKey,
- pointRadius,
- pointRadiusUnit,
- renderWhileDragging,
- rgb,
- hasCustomMetric,
- bounds,
- } = this.props;
- const { viewport } = this.state;
- const isDragging =
- viewport.isDragging === undefined ? false : viewport.isDragging;
-
- // Compute the clusters based on the original bounds and current zoom
level. Note when zoom/pan
- // to an area outside of the original bounds, no additional queries are
made to the backend to
- // retrieve additional data.
- // add this variable to widen the visible area
- const offsetHorizontal = (width * 0.5) / 100;
- const offsetVertical = (height * 0.5) / 100;
- const bbox = [
- bounds[0][0] - offsetHorizontal,
- bounds[0][1] - offsetVertical,
- bounds[1][0] + offsetHorizontal,
- bounds[1][1] + offsetVertical,
- ];
- const clusters = clusterer.getClusters(bbox, Math.round(viewport.zoom));
-
- return (
- <MapGL
+ const isDragging =
Review Comment:
It's equivalent to `Boolean()`
```suggestion
const isDragging = Boolean(viewport.isDragging);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
