sadpandajoe commented on code in PR #39760:
URL: https://github.com/apache/superset/pull/39760#discussion_r4377999381
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx:
##########
@@ -158,6 +173,35 @@ export const createWfsLayer = async (wfsLayerConf:
WfsLayerConf) => {
});
};
+/**
+ * Create a DATA layer.
+ *
+ * @param dataLayerConf The layer configuration
+ *
+ * @returns The created DATA layer
+ */
+export const createDataLayer = async (dataLayerConf: DataLayerConf) => {
+ const { attribution, style } = dataLayerConf;
+ const dataSource = new VectorSource({
+ attributions: attribution,
Review Comment:
`createDataLayer` passes `attribution` to the `VectorSource` unescaped,
while the WMS, XYZ and WFS creators in this file all go through
`escapeAttribution` because OpenLayers renders attributions as HTML. A chart
owner can save `layer_configs` with `type: 'DATA'` and an attribution like
`<img src=x onerror=...>` (via the UI field or the chart API), and the script
runs for anyone who opens the chart or a dashboard containing it. Should this
use `escapeAttribution(attribution)` like the other layer types, with a
matching case next to the existing escape tests in `layerUtil.test.ts`?
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts:
##########
@@ -17,44 +17,63 @@
* under the License.
*/
import { ChartProps, getChartTransformPropsRegistry } from '@superset-ui/core';
-import {
- getChartConfigs,
- parseSelectedChart,
-} from '../util/transformPropsUtil';
+import { getChartConfigs, parseSelectedChart } from
'../util/transformPropsUtil';
+import { ChartConfig } from '../types';
export default function transformProps(chartProps: ChartProps) {
const { width, height, formData, hooks, theme } = chartProps;
const {
geomColumn,
+ geomFormat,
selectedChart: selectedChartString,
chartSize,
layerConfigs,
+ mapExtentPadding,
+ mapMaxExtent,
Review Comment:
`mapMaxExtent` (and `geomFormat`, `minZoom`, `maxZoom`, `mapExtentPadding`)
are read straight from `formData` with no fallback. Charts saved before this
change have none of these keys in their stored params, and dashboards render
from the stored params rather than the control defaults that Explore applies.
For such a chart `OlChartMap` destructures `extentMode` from an undefined
`mapMaxExtent` and throws on mount, and a missing `geomFormat` sends GeoJSON
data through the WKT path so the map comes up empty. Should `transformProps`
default these values (for example to the control defaults) so existing saved
charts keep rendering?
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx:
##########
@@ -20,21 +20,67 @@ import { useEffect, useState } from 'react';
import { useSelector } from 'react-redux';
import Point from 'ol/geom/Point';
-import { View } from 'ol';
+import { Map, View } from 'ol';
import BaseEvent from 'ol/events/Event';
+import GeoJSON from 'ol/format/GeoJSON';
import { unByKey } from 'ol/Observable';
-import { toLonLat } from 'ol/proj';
+import { toLonLat, transformExtent } from 'ol/proj';
import { debounce } from 'lodash-es';
-import { fitMapToCharts } from '../util/mapUtil';
+import { fitMapToData } from '../util/mapUtil';
import { ChartLayer } from './ChartLayer';
import { createLayer } from '../util/layerUtil';
import {
ChartConfig,
LayerConf,
+ MapMaxExtentConfigs,
MapViewConfigs,
OlChartMapProps,
} from '../types';
import { isChartConfigEqual } from '../util/chartUtil';
+import {
+ getExtentFromFeatures,
+ getMapExtentPadding,
+} from '../util/geometryUtil';
+
+/**
+ * Computes a transformed extent from a MapMaxExtentConfigs (if CUSTOM mode)
+ * and applies it by setting a new view on the map.
+ */
+const applyMapExtentView = (
+ olMap: Map,
+ mapMaxExtentConf: MapMaxExtentConfigs,
+) => {
+ const { extentMode, fixedMaxX, fixedMaxY, fixedMinX, fixedMinY } =
+ mapMaxExtentConf;
+ const view = olMap.getView();
+ const center = view.getCenter();
+ const zoom = view.getZoom();
+ let extent;
+
+ if (
+ extentMode === 'CUSTOM' &&
+ fixedMaxX !== undefined &&
+ fixedMaxY !== undefined &&
+ fixedMinX !== undefined &&
+ fixedMinY !== undefined
+ ) {
+ extent = transformExtent(
+ [fixedMinX, fixedMinY, fixedMaxX, fixedMaxY],
+ 'EPSG:4326',
+ 'EPSG:3857',
+ );
+ }
+
+ olMap.setView(
Review Comment:
This replaces the map's `View` whenever `currentMapMaxExtent` changes, but
the zoom-sync listener further down (the `change:resolution` handler, deps
`[olMap, setControlValue, chartSize]`) stays attached to the view it captured
earlier. After switching Max. Extent to CUSTOM or clicking "Use current extent"
and then zooming, that handler never sees the zoom event, so
`chart_size.configs.zoom` stops following the map and the zoom-based size
controls compute against a stale zoom. Should that effect also re-subscribe
when the extent (and therefore the view) changes?
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx:
##########
@@ -0,0 +1,155 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ControlHeader } from '@superset-ui/chart-controls';
+import { t } from '@apache-superset/core/translation';
+import { css, styled } from '@apache-superset/core/theme';
+import { Button, Radio, RadioChangeEvent } from '@superset-ui/core/components';
+import { FC } from 'react';
+import { mix } from 'polished';
+import {
+ MapMaxExtentConfigs,
+ MapMaxExtentConfigsControlProps,
+} from '../../types';
+import MapMaxExtentTag from './MapMaxExtentTag';
+
+export const StyledMaxExtentButton = styled(Button)`
+ ${({ theme }) => css`
+ flex: 1;
+ margin-right: 4px;
+ line-height: 1.5715;
+ border-radius: ${theme.borderRadius}px;
+ background-color: ${theme.colorPrimaryBg};
+ color: ${theme.colorPrimaryText};
+ font-size: ${theme.fontSizeSM}px;
+ font-weight: ${theme.fontWeightStrong};
+ text-transform: uppercase;
+ min-width: ${theme.sizeUnit * 36}px;
+ min-height: ${theme.padding}px;
+ box-shadow: none;
+ border-width: 0px;
+ border-style: none;
+ border-color: transparent;
+ margin-left: 9px;
+ margin-top: 10px;
+ &:hover {
+ background-color: ${mix(0.1, theme.colorPrimary, theme.colorPrimaryBg)};
+ color: ${theme.colorPrimaryText};
+ }
+ `}
+`;
+
+export const StyledMaxExtentTag = styled(MapMaxExtentTag)`
+ ${() => css`
+ margin-left: 9px;
+ `}
+`;
+
+export const MapMaxExtentViewControl: FC<MapMaxExtentConfigsControlProps> = ({
+ value,
+ onChange = () => {},
+ name,
+ label,
+ description,
+ renderTrigger,
+ hovered,
+ validationErrors,
+}) => {
+ const isCustomMode = () => value?.extentMode === 'CUSTOM';
+
+ const onModeChange = (e: RadioChangeEvent) => {
+ if (!value) return;
+
+ const newMode = e.target.value;
+ let changedValue: MapMaxExtentConfigs = {
+ ...value,
+ extentMode: newMode,
+ };
+ if (newMode === 'NONE') {
+ changedValue = {
+ ...value,
+ fixedMaxX: undefined,
+ fixedMaxY: undefined,
+ fixedMinX: undefined,
+ fixedMinY: undefined,
+ extentMode: newMode,
+ };
+ }
+
+ onChange(changedValue);
+ };
+
+ const onButtonClick = () => {
Review Comment:
The capture/clear flow in this control and `OlChartMap` has no
component-level coverage: `onButtonClick` copies the live `minX/minY/maxX/maxY`
into the fixed bounds, and switching back to NONE has to clear them, but the
current utility tests never render the control. A regression where the capture
copies stale bounds, or NONE leaves the fixed values behind (so the map stays
clamped), would pass the suite. Could you add a test that renders the control
with live bounds, clicks "Use current extent" and asserts `onChange` receives
the fixed values, then selects NONE and asserts they are `undefined`?
##########
superset-frontend/plugins/plugin-chart-cartodiagram/package.json:
##########
@@ -31,8 +31,9 @@
"dependencies": {
"@types/geojson": "^7946.0.16",
"geojson": "^0.5.0",
- "lodash": "^4.18.1",
- "lodash-es": "^4.18.1"
+ "geostyler-legend": "^5.2.1",
Review Comment:
`geostyler-legend` is added as a runtime dependency (with matching webpack
resolve/babel-exclude entries and a large lockfile addition), but nothing under
the plugin or `superset-frontend/src` imports it. Fresh installs pull it and
its d3 tree for no functionality. Is there a follow-up that uses it, or can it
be dropped from this PR?
--
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]
