sadpandajoe commented on code in PR #39760:
URL: https://github.com/apache/superset/pull/39760#discussion_r4399781655
##########
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:
Unlike the WMS/WFS/XYZ constructors above, this branch passes `attribution`
to the source without `escapeAttribution()`. OpenLayers renders attributions
through `innerHTML`, so a chart saved through the API with
`{"type":"DATA","attribution":"<img src=x onerror=...>"}` (the UI selector for
DATA being disabled doesn't stop API-supplied `layer_configs`) would inject
markup for every viewer wherever CSP allows inline handlers. Should this use
`escapeAttribution(attribution)` like the other layer types?
```suggestion
attributions: escapeAttribution(attribution),
```
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx:
##########
@@ -196,6 +325,10 @@ export const OlChartMap = (props: OlChartMapProps) => {
addLayers(layerConfigs);
}, [olMap, layerConfigs]);
+ useEffect(() => {
+ applyMapExtentView(olMap, currentMapMaxExtent);
Review Comment:
`applyMapExtentView` calls `olMap.setView(new View(...))`, but the
`change:resolution` listener in the chart-size effect further down is bound to
`olMap.getView()` at the time that effect ran, and its deps (`olMap`,
`setControlValue`, `chartSize`) don't change when the extent does. After
toggling Max. Extent (NONE/CUSTOM) and then zooming, the old view fires the
events, so `chart_size.configs.zoom` stops updating and later size edits use a
stale reference zoom. Could the listener be re-bound when the view is replaced
(e.g. include `currentMapMaxExtent` in that effect's deps)?
--
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]
