Copilot commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2470443245
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -83,112 +106,200 @@ function CountryMap(element, props) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
- const backgroundRect = svg
- .append('rect')
- .attr('class', 'background')
- .attr('width', width)
- .attr('height', height);
+
const g = svg.append('g');
const mapLayer = g.append('g').classed('map-layer', true);
- const textLayer = g
+ const textLayer = svg
.append('g')
- .classed('text-layer', true)
+ .attr('class', 'text-layer')
.attr('transform', `translate(${width / 2}, 45)`);
- const bigText = textLayer.append('text').classed('big-text', true);
+ const bigText = textLayer
+ .append('text')
+ .classed('big-text', true)
+ .style('font-size', '18px');
const resultText = textLayer
.append('text')
.classed('result-text', true)
- .attr('dy', '1em');
-
- let centered;
-
- const clicked = function clicked(d) {
- const hasCenter = d && centered !== d;
- let x;
- let y;
- let k;
- const halfWidth = width / 2;
- const halfHeight = height / 2;
-
- if (hasCenter) {
- const centroid = path.centroid(d);
- [x, y] = centroid;
- k = 4;
- centered = d;
- } else {
- x = halfWidth;
- y = halfHeight;
- k = 1;
- centered = null;
- }
+ .attr('dy', '1em')
+ .style('font-size', '26px');
+
+ // Cross-filter support
+ const getCrossFilterDataMask = source => {
+ const selected = filterState.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
-
`translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
- textLayer
- .style('opacity', 0)
- .attr(
- 'transform',
- `translate(0,0)translate(${x},${hasCenter ? y - 5 : 45})`,
- )
- .transition()
- .duration(750)
- .style('opacity', 1);
- bigText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 6 : 16);
- resultText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 16 : 24);
+ const isSelected = selected.includes(iso);
+ const values = isSelected ? [] : [iso];
+
+ return {
+ dataMask: {
+ extraFormData: {
+ filters: values.length
+ ? [{ col: entity, op: 'IN', val: values }]
+ : [],
+ },
+ filterState: {
+ value: values.length ? values : null,
+ selectedValues: values.length ? values : null,
+ },
+ },
+ isCurrentValueSelected: isSelected,
+ };
};
- backgroundRect.on('click', clicked);
+ // Handle right-click context menu
+ const handleContextMenu = feature => {
+ const pointerEvent = d3.event;
+
+ // Only prevent default if we have a context menu handler
+ if (typeof onContextMenu === 'function') {
+ pointerEvent?.preventDefault();
+ }
+
+ const iso = feature?.properties?.ISO;
+ if (!iso || typeof onContextMenu !== 'function') return;
+
+ const drillVal = iso;
+ const drillToDetailFilters = [
+ { col: entity, op: '==', val: drillVal, formattedVal: drillVal },
+ ];
+ const drillByFilters = [{ col: entity, op: '==', val: drillVal }];
- const selectAndDisplayNameOfRegion = function selectAndDisplayNameOfRegion(
- feature,
- ) {
+ onContextMenu(pointerEvent.clientX, pointerEvent.clientY, {
+ drillToDetail: drillToDetailFilters,
+ crossFilter: getCrossFilterDataMask(feature),
+ drillBy: { filters: drillByFilters, groupbyFieldName: 'entity' },
+ });
+ };
+
+ const selectAndDisplayNameOfRegion = feature => {
let name = '';
- if (feature && feature.properties) {
- if (feature.properties.ID_2) {
- name = feature.properties.NAME_2;
- } else {
- name = feature.properties.NAME_1;
- }
+ if (feature?.properties) {
+ name = feature.properties.NAME_2 || feature.properties.NAME_1 || '';
}
bigText.text(name);
};
- const updateMetrics = function updateMetrics(region) {
- if (region.length > 0) {
- resultText.text(format(region[0].metric));
- }
+ const updateMetrics = regionRows => {
+ if (regionRows?.length > 0) resultText.text(format(regionRows[0].metric));
+ else resultText.text('');
};
- const mouseenter = function mouseenter(d) {
- // Darken color
+ const mouseenter = function (d) {
let c = colorFn(d);
- if (c !== 'none') {
- c = d3.rgb(c).darker().toString();
- }
+ if (c && c !== 'none') c = d3.rgb(c).darker().toString();
d3.select(this).style('fill', c);
selectAndDisplayNameOfRegion(d);
- const result = data.filter(
- region => region.country_id === d.properties.ISO,
- );
+ const result = data.filter(r => r.country_id === d?.properties?.ISO);
updateMetrics(result);
};
- const mouseout = function mouseout() {
- d3.select(this).style('fill', colorFn);
+ const mouseout = function () {
+ d3.select(this).style('fill', d => colorFn(d));
bigText.text('');
resultText.text('');
};
+ // Zoom with panning bounds
+ const zoom = d3.behavior
+ .zoom()
+ .scaleExtent([1, 4])
+ .on('zoom', () => {
+ const { translate, scale } = d3.event; // [tx, ty]
+ let [tx, ty] = translate;
+
+ const scaledW = width * scale;
+ const scaledH = height * scale;
+ const minX = Math.min(0, width - scaledW);
+ const maxX = 0;
+ const minY = Math.min(0, height - scaledH);
+ const maxY = 0;
+
+ // clamp
+ tx = Math.max(Math.min(tx, maxX), minX);
+ ty = Math.max(Math.min(ty, maxY), minY);
+
+ g.attr('transform', `translate(${tx}, ${ty}) scale(${scale})`);
+ zoomStates[chartKey] = { scale, translate: [tx, ty] };
+ });
+
+ d3.select(svg.node()).call(zoom);
+
+ // Restore
+ if (zoomStates[chartKey]) {
+ const { scale, translate } = zoomStates[chartKey];
+ zoom.scale(scale).translate(translate);
+ g.attr(
+ 'transform',
+ `translate(${translate[0]}, ${translate[1]}) scale(${scale})`,
+ );
+ }
+
+ // Visual highlighting for selected regions
+ function highlightSelectedRegion() {
+ const selectedValues = filterState.selectedValues?.length
+ ? filterState.selectedValues
+ : [];
+
+ mapLayer
+ .selectAll('path.region')
+ .style('fill-opacity', d => {
+ const iso = d?.properties?.ISO;
+ return selectedValues.length === 0 || selectedValues.includes(iso)
+ ? 1
+ : 0.3;
+ })
+ .style('stroke', d => {
+ const iso = d?.properties?.ISO;
+ return selectedValues.includes(iso) ? '#222' : null;
+ })
+ .style('stroke-width', d => {
+ const iso = d?.properties?.ISO;
+ return selectedValues.includes(iso) ? '1.5px' : '0.5px';
+ });
+ }
+
+ // Click handler
+ const handleClick = feature => {
+ if (!emitCrossFilters || typeof setDataMask !== 'function') return;
+ const iso = feature?.properties?.ISO;
+ if (!iso) return;
+
+ const baseline = filterState.selectedValues || [];
Review Comment:
Potential runtime error: `filterState.selectedValues` will throw an error if
`filterState` is `undefined` or `null`. Consider using optional chaining:
`const baseline = filterState?.selectedValues || [];`
```suggestion
const baseline = filterState?.selectedValues || [];
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -83,112 +106,200 @@ function CountryMap(element, props) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
- const backgroundRect = svg
- .append('rect')
- .attr('class', 'background')
- .attr('width', width)
- .attr('height', height);
+
const g = svg.append('g');
const mapLayer = g.append('g').classed('map-layer', true);
- const textLayer = g
+ const textLayer = svg
.append('g')
- .classed('text-layer', true)
+ .attr('class', 'text-layer')
.attr('transform', `translate(${width / 2}, 45)`);
- const bigText = textLayer.append('text').classed('big-text', true);
+ const bigText = textLayer
+ .append('text')
+ .classed('big-text', true)
+ .style('font-size', '18px');
const resultText = textLayer
.append('text')
.classed('result-text', true)
- .attr('dy', '1em');
-
- let centered;
-
- const clicked = function clicked(d) {
- const hasCenter = d && centered !== d;
- let x;
- let y;
- let k;
- const halfWidth = width / 2;
- const halfHeight = height / 2;
-
- if (hasCenter) {
- const centroid = path.centroid(d);
- [x, y] = centroid;
- k = 4;
- centered = d;
- } else {
- x = halfWidth;
- y = halfHeight;
- k = 1;
- centered = null;
- }
+ .attr('dy', '1em')
+ .style('font-size', '26px');
+
+ // Cross-filter support
+ const getCrossFilterDataMask = source => {
+ const selected = filterState.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
-
`translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
- textLayer
- .style('opacity', 0)
- .attr(
- 'transform',
- `translate(0,0)translate(${x},${hasCenter ? y - 5 : 45})`,
- )
- .transition()
- .duration(750)
- .style('opacity', 1);
- bigText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 6 : 16);
- resultText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 16 : 24);
+ const isSelected = selected.includes(iso);
+ const values = isSelected ? [] : [iso];
+
+ return {
+ dataMask: {
+ extraFormData: {
+ filters: values.length
+ ? [{ col: entity, op: 'IN', val: values }]
+ : [],
+ },
+ filterState: {
+ value: values.length ? values : null,
+ selectedValues: values.length ? values : null,
+ },
+ },
+ isCurrentValueSelected: isSelected,
+ };
};
Review Comment:
Missing safety check: If `entity` prop is undefined or null, all filter
operations in this function will fail. Add a guard check at the beginning: `if
(!entity) return undefined;`
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -83,112 +106,200 @@ function CountryMap(element, props) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
- const backgroundRect = svg
- .append('rect')
- .attr('class', 'background')
- .attr('width', width)
- .attr('height', height);
+
const g = svg.append('g');
const mapLayer = g.append('g').classed('map-layer', true);
- const textLayer = g
+ const textLayer = svg
.append('g')
- .classed('text-layer', true)
+ .attr('class', 'text-layer')
.attr('transform', `translate(${width / 2}, 45)`);
- const bigText = textLayer.append('text').classed('big-text', true);
+ const bigText = textLayer
+ .append('text')
+ .classed('big-text', true)
+ .style('font-size', '18px');
const resultText = textLayer
.append('text')
.classed('result-text', true)
- .attr('dy', '1em');
-
- let centered;
-
- const clicked = function clicked(d) {
- const hasCenter = d && centered !== d;
- let x;
- let y;
- let k;
- const halfWidth = width / 2;
- const halfHeight = height / 2;
-
- if (hasCenter) {
- const centroid = path.centroid(d);
- [x, y] = centroid;
- k = 4;
- centered = d;
- } else {
- x = halfWidth;
- y = halfHeight;
- k = 1;
- centered = null;
- }
+ .attr('dy', '1em')
+ .style('font-size', '26px');
+
+ // Cross-filter support
+ const getCrossFilterDataMask = source => {
+ const selected = filterState.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
-
`translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
- textLayer
- .style('opacity', 0)
- .attr(
- 'transform',
- `translate(0,0)translate(${x},${hasCenter ? y - 5 : 45})`,
- )
- .transition()
- .duration(750)
- .style('opacity', 1);
- bigText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 6 : 16);
- resultText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 16 : 24);
+ const isSelected = selected.includes(iso);
+ const values = isSelected ? [] : [iso];
+
+ return {
+ dataMask: {
+ extraFormData: {
+ filters: values.length
+ ? [{ col: entity, op: 'IN', val: values }]
+ : [],
+ },
+ filterState: {
+ value: values.length ? values : null,
+ selectedValues: values.length ? values : null,
+ },
+ },
+ isCurrentValueSelected: isSelected,
+ };
};
- backgroundRect.on('click', clicked);
+ // Handle right-click context menu
+ const handleContextMenu = feature => {
+ const pointerEvent = d3.event;
+
+ // Only prevent default if we have a context menu handler
+ if (typeof onContextMenu === 'function') {
+ pointerEvent?.preventDefault();
+ }
+
+ const iso = feature?.properties?.ISO;
+ if (!iso || typeof onContextMenu !== 'function') return;
+
+ const drillVal = iso;
+ const drillToDetailFilters = [
+ { col: entity, op: '==', val: drillVal, formattedVal: drillVal },
+ ];
+ const drillByFilters = [{ col: entity, op: '==', val: drillVal }];
- const selectAndDisplayNameOfRegion = function selectAndDisplayNameOfRegion(
- feature,
- ) {
+ onContextMenu(pointerEvent.clientX, pointerEvent.clientY, {
+ drillToDetail: drillToDetailFilters,
+ crossFilter: getCrossFilterDataMask(feature),
+ drillBy: { filters: drillByFilters, groupbyFieldName: 'entity' },
+ });
+ };
+
+ const selectAndDisplayNameOfRegion = feature => {
let name = '';
- if (feature && feature.properties) {
- if (feature.properties.ID_2) {
- name = feature.properties.NAME_2;
- } else {
- name = feature.properties.NAME_1;
- }
+ if (feature?.properties) {
+ name = feature.properties.NAME_2 || feature.properties.NAME_1 || '';
}
bigText.text(name);
};
- const updateMetrics = function updateMetrics(region) {
- if (region.length > 0) {
- resultText.text(format(region[0].metric));
- }
+ const updateMetrics = regionRows => {
+ if (regionRows?.length > 0) resultText.text(format(regionRows[0].metric));
+ else resultText.text('');
};
- const mouseenter = function mouseenter(d) {
- // Darken color
+ const mouseenter = function (d) {
let c = colorFn(d);
- if (c !== 'none') {
- c = d3.rgb(c).darker().toString();
- }
+ if (c && c !== 'none') c = d3.rgb(c).darker().toString();
d3.select(this).style('fill', c);
selectAndDisplayNameOfRegion(d);
- const result = data.filter(
- region => region.country_id === d.properties.ISO,
- );
+ const result = data.filter(r => r.country_id === d?.properties?.ISO);
updateMetrics(result);
};
- const mouseout = function mouseout() {
- d3.select(this).style('fill', colorFn);
+ const mouseout = function () {
+ d3.select(this).style('fill', d => colorFn(d));
bigText.text('');
resultText.text('');
};
+ // Zoom with panning bounds
+ const zoom = d3.behavior
+ .zoom()
+ .scaleExtent([1, 4])
+ .on('zoom', () => {
+ const { translate, scale } = d3.event; // [tx, ty]
+ let [tx, ty] = translate;
+
+ const scaledW = width * scale;
+ const scaledH = height * scale;
+ const minX = Math.min(0, width - scaledW);
+ const maxX = 0;
+ const minY = Math.min(0, height - scaledH);
+ const maxY = 0;
+
+ // clamp
+ tx = Math.max(Math.min(tx, maxX), minX);
+ ty = Math.max(Math.min(ty, maxY), minY);
+
+ g.attr('transform', `translate(${tx}, ${ty}) scale(${scale})`);
+ zoomStates[chartKey] = { scale, translate: [tx, ty] };
+ });
+
+ d3.select(svg.node()).call(zoom);
+
+ // Restore
+ if (zoomStates[chartKey]) {
+ const { scale, translate } = zoomStates[chartKey];
+ zoom.scale(scale).translate(translate);
+ g.attr(
+ 'transform',
+ `translate(${translate[0]}, ${translate[1]}) scale(${scale})`,
+ );
+ }
+
+ // Visual highlighting for selected regions
+ function highlightSelectedRegion() {
+ const selectedValues = filterState.selectedValues?.length
+ ? filterState.selectedValues
+ : [];
+
+ mapLayer
+ .selectAll('path.region')
+ .style('fill-opacity', d => {
+ const iso = d?.properties?.ISO;
+ return selectedValues.length === 0 || selectedValues.includes(iso)
+ ? 1
+ : 0.3;
+ })
+ .style('stroke', d => {
+ const iso = d?.properties?.ISO;
+ return selectedValues.includes(iso) ? '#222' : null;
+ })
+ .style('stroke-width', d => {
+ const iso = d?.properties?.ISO;
+ return selectedValues.includes(iso) ? '1.5px' : '0.5px';
+ });
+ }
+
+ // Click handler
+ const handleClick = feature => {
+ if (!emitCrossFilters || typeof setDataMask !== 'function') return;
Review Comment:
Missing safety check: If `entity` prop is undefined or null, the filter
operations will fail. Add a guard check at the beginning of the function: `if
(!entity || typeof setDataMask !== 'function') return;`
```suggestion
if (!emitCrossFilters || typeof setDataMask !== 'function' || !entity)
return;
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -70,7 +88,12 @@ function CountryMap(element, props) {
? colorScale(d.country_id, sliceId)
: linearColorScale(d.metric);
});
- const colorFn = d => colorMap[d.properties.ISO] || 'none';
+
+ const colorFn = feature => {
+ if (!feature?.properties) return 'none';
+ const iso = feature.properties.ISO;
+ return colorMap[iso] || '#FFFEFE';
Review Comment:
The default color `'#FFFEFE'` for regions without data is nearly white and
may be difficult to distinguish from the background, especially in light
themes. Consider using a more neutral color like `'#E0E0E0'` (light gray) to
make it clearer which regions have no data.
```suggestion
return colorMap[iso] || '#E0E0E0';
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -83,112 +106,200 @@ function CountryMap(element, props) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
- const backgroundRect = svg
- .append('rect')
- .attr('class', 'background')
- .attr('width', width)
- .attr('height', height);
+
const g = svg.append('g');
const mapLayer = g.append('g').classed('map-layer', true);
- const textLayer = g
+ const textLayer = svg
.append('g')
- .classed('text-layer', true)
+ .attr('class', 'text-layer')
.attr('transform', `translate(${width / 2}, 45)`);
- const bigText = textLayer.append('text').classed('big-text', true);
+ const bigText = textLayer
+ .append('text')
+ .classed('big-text', true)
+ .style('font-size', '18px');
const resultText = textLayer
.append('text')
.classed('result-text', true)
- .attr('dy', '1em');
-
- let centered;
-
- const clicked = function clicked(d) {
- const hasCenter = d && centered !== d;
- let x;
- let y;
- let k;
- const halfWidth = width / 2;
- const halfHeight = height / 2;
-
- if (hasCenter) {
- const centroid = path.centroid(d);
- [x, y] = centroid;
- k = 4;
- centered = d;
- } else {
- x = halfWidth;
- y = halfHeight;
- k = 1;
- centered = null;
- }
+ .attr('dy', '1em')
+ .style('font-size', '26px');
+
+ // Cross-filter support
+ const getCrossFilterDataMask = source => {
+ const selected = filterState.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
-
`translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
- textLayer
- .style('opacity', 0)
- .attr(
- 'transform',
- `translate(0,0)translate(${x},${hasCenter ? y - 5 : 45})`,
- )
- .transition()
- .duration(750)
- .style('opacity', 1);
- bigText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 6 : 16);
- resultText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 16 : 24);
+ const isSelected = selected.includes(iso);
+ const values = isSelected ? [] : [iso];
+
+ return {
+ dataMask: {
+ extraFormData: {
+ filters: values.length
+ ? [{ col: entity, op: 'IN', val: values }]
+ : [],
+ },
+ filterState: {
+ value: values.length ? values : null,
+ selectedValues: values.length ? values : null,
+ },
+ },
+ isCurrentValueSelected: isSelected,
+ };
};
- backgroundRect.on('click', clicked);
+ // Handle right-click context menu
+ const handleContextMenu = feature => {
+ const pointerEvent = d3.event;
+
+ // Only prevent default if we have a context menu handler
+ if (typeof onContextMenu === 'function') {
+ pointerEvent?.preventDefault();
+ }
+
+ const iso = feature?.properties?.ISO;
+ if (!iso || typeof onContextMenu !== 'function') return;
+
+ const drillVal = iso;
+ const drillToDetailFilters = [
+ { col: entity, op: '==', val: drillVal, formattedVal: drillVal },
+ ];
+ const drillByFilters = [{ col: entity, op: '==', val: drillVal }];
- const selectAndDisplayNameOfRegion = function selectAndDisplayNameOfRegion(
- feature,
- ) {
+ onContextMenu(pointerEvent.clientX, pointerEvent.clientY, {
+ drillToDetail: drillToDetailFilters,
+ crossFilter: getCrossFilterDataMask(feature),
+ drillBy: { filters: drillByFilters, groupbyFieldName: 'entity' },
Review Comment:
The `drillBy` configuration uses a hardcoded string `'entity'` for
`groupbyFieldName`, but it should use the actual `entity` variable from props
which represents the column name being used. Change to: `drillBy: { filters:
drillByFilters, groupbyFieldName: entity }`
```suggestion
drillBy: { filters: drillByFilters, groupbyFieldName: entity },
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -83,112 +106,200 @@ function CountryMap(element, props) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
- const backgroundRect = svg
- .append('rect')
- .attr('class', 'background')
- .attr('width', width)
- .attr('height', height);
+
const g = svg.append('g');
const mapLayer = g.append('g').classed('map-layer', true);
- const textLayer = g
+ const textLayer = svg
.append('g')
- .classed('text-layer', true)
+ .attr('class', 'text-layer')
.attr('transform', `translate(${width / 2}, 45)`);
- const bigText = textLayer.append('text').classed('big-text', true);
+ const bigText = textLayer
+ .append('text')
+ .classed('big-text', true)
+ .style('font-size', '18px');
const resultText = textLayer
.append('text')
.classed('result-text', true)
- .attr('dy', '1em');
-
- let centered;
-
- const clicked = function clicked(d) {
- const hasCenter = d && centered !== d;
- let x;
- let y;
- let k;
- const halfWidth = width / 2;
- const halfHeight = height / 2;
-
- if (hasCenter) {
- const centroid = path.centroid(d);
- [x, y] = centroid;
- k = 4;
- centered = d;
- } else {
- x = halfWidth;
- y = halfHeight;
- k = 1;
- centered = null;
- }
+ .attr('dy', '1em')
+ .style('font-size', '26px');
+
+ // Cross-filter support
+ const getCrossFilterDataMask = source => {
+ const selected = filterState.selectedValues || [];
Review Comment:
Potential runtime error: `filterState.selectedValues` will throw an error if
`filterState` is `undefined` or `null`. Consider using optional chaining:
`const selected = filterState?.selectedValues || [];`
```suggestion
const selected = filterState?.selectedValues || [];
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -41,27 +41,45 @@ const propTypes = {
linearColorScheme: PropTypes.string,
mapBaseUrl: PropTypes.string,
numberFormat: PropTypes.string,
+ onContextMenu: PropTypes.func,
+ emitCrossFilters: PropTypes.bool,
+ setDataMask: PropTypes.func,
+ filterState: PropTypes.object,
+ entity: PropTypes.string,
+ sliceId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
};
const maps = {};
+// Store zoom state per chart instance
+const zoomStates = {};
Review Comment:
The comment "Store zoom state per chart instance" describes the purpose of
`zoomStates` but there's no cleanup mechanism for this global object. When
charts are removed, their zoom states will persist in memory causing a memory
leak. Consider adding a cleanup function or using a WeakMap if possible.
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -83,112 +106,200 @@ function CountryMap(element, props) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
- const backgroundRect = svg
- .append('rect')
- .attr('class', 'background')
- .attr('width', width)
- .attr('height', height);
+
const g = svg.append('g');
const mapLayer = g.append('g').classed('map-layer', true);
- const textLayer = g
+ const textLayer = svg
.append('g')
- .classed('text-layer', true)
+ .attr('class', 'text-layer')
.attr('transform', `translate(${width / 2}, 45)`);
- const bigText = textLayer.append('text').classed('big-text', true);
+ const bigText = textLayer
+ .append('text')
+ .classed('big-text', true)
+ .style('font-size', '18px');
const resultText = textLayer
.append('text')
.classed('result-text', true)
- .attr('dy', '1em');
-
- let centered;
-
- const clicked = function clicked(d) {
- const hasCenter = d && centered !== d;
- let x;
- let y;
- let k;
- const halfWidth = width / 2;
- const halfHeight = height / 2;
-
- if (hasCenter) {
- const centroid = path.centroid(d);
- [x, y] = centroid;
- k = 4;
- centered = d;
- } else {
- x = halfWidth;
- y = halfHeight;
- k = 1;
- centered = null;
- }
+ .attr('dy', '1em')
+ .style('font-size', '26px');
+
+ // Cross-filter support
+ const getCrossFilterDataMask = source => {
+ const selected = filterState.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
-
`translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
- textLayer
- .style('opacity', 0)
- .attr(
- 'transform',
- `translate(0,0)translate(${x},${hasCenter ? y - 5 : 45})`,
- )
- .transition()
- .duration(750)
- .style('opacity', 1);
- bigText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 6 : 16);
- resultText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 16 : 24);
+ const isSelected = selected.includes(iso);
+ const values = isSelected ? [] : [iso];
+
+ return {
+ dataMask: {
+ extraFormData: {
+ filters: values.length
+ ? [{ col: entity, op: 'IN', val: values }]
+ : [],
+ },
+ filterState: {
+ value: values.length ? values : null,
+ selectedValues: values.length ? values : null,
+ },
+ },
+ isCurrentValueSelected: isSelected,
+ };
};
- backgroundRect.on('click', clicked);
+ // Handle right-click context menu
+ const handleContextMenu = feature => {
+ const pointerEvent = d3.event;
+
+ // Only prevent default if we have a context menu handler
+ if (typeof onContextMenu === 'function') {
+ pointerEvent?.preventDefault();
+ }
+
+ const iso = feature?.properties?.ISO;
+ if (!iso || typeof onContextMenu !== 'function') return;
+
+ const drillVal = iso;
+ const drillToDetailFilters = [
+ { col: entity, op: '==', val: drillVal, formattedVal: drillVal },
+ ];
+ const drillByFilters = [{ col: entity, op: '==', val: drillVal }];
- const selectAndDisplayNameOfRegion = function selectAndDisplayNameOfRegion(
- feature,
- ) {
+ onContextMenu(pointerEvent.clientX, pointerEvent.clientY, {
+ drillToDetail: drillToDetailFilters,
+ crossFilter: getCrossFilterDataMask(feature),
+ drillBy: { filters: drillByFilters, groupbyFieldName: 'entity' },
+ });
+ };
+
+ const selectAndDisplayNameOfRegion = feature => {
let name = '';
- if (feature && feature.properties) {
- if (feature.properties.ID_2) {
- name = feature.properties.NAME_2;
- } else {
- name = feature.properties.NAME_1;
- }
+ if (feature?.properties) {
+ name = feature.properties.NAME_2 || feature.properties.NAME_1 || '';
}
bigText.text(name);
};
- const updateMetrics = function updateMetrics(region) {
- if (region.length > 0) {
- resultText.text(format(region[0].metric));
- }
+ const updateMetrics = regionRows => {
+ if (regionRows?.length > 0) resultText.text(format(regionRows[0].metric));
+ else resultText.text('');
};
- const mouseenter = function mouseenter(d) {
- // Darken color
+ const mouseenter = function (d) {
let c = colorFn(d);
- if (c !== 'none') {
- c = d3.rgb(c).darker().toString();
- }
+ if (c && c !== 'none') c = d3.rgb(c).darker().toString();
d3.select(this).style('fill', c);
selectAndDisplayNameOfRegion(d);
- const result = data.filter(
- region => region.country_id === d.properties.ISO,
- );
+ const result = data.filter(r => r.country_id === d?.properties?.ISO);
updateMetrics(result);
};
- const mouseout = function mouseout() {
- d3.select(this).style('fill', colorFn);
+ const mouseout = function () {
+ d3.select(this).style('fill', d => colorFn(d));
bigText.text('');
resultText.text('');
};
+ // Zoom with panning bounds
+ const zoom = d3.behavior
+ .zoom()
+ .scaleExtent([1, 4])
+ .on('zoom', () => {
+ const { translate, scale } = d3.event; // [tx, ty]
+ let [tx, ty] = translate;
+
+ const scaledW = width * scale;
+ const scaledH = height * scale;
+ const minX = Math.min(0, width - scaledW);
+ const maxX = 0;
+ const minY = Math.min(0, height - scaledH);
+ const maxY = 0;
+
+ // clamp
+ tx = Math.max(Math.min(tx, maxX), minX);
+ ty = Math.max(Math.min(ty, maxY), minY);
+
+ g.attr('transform', `translate(${tx}, ${ty}) scale(${scale})`);
+ zoomStates[chartKey] = { scale, translate: [tx, ty] };
+ });
Review Comment:
Performance issue: The zoom event handler runs on every zoom/pan event
without throttling or debouncing. For large maps with many regions, this could
cause performance issues. Consider throttling the zoom updates using
`d3.event.type === 'zoomend'` or implementing a throttle mechanism.
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -83,112 +106,200 @@ function CountryMap(element, props) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
- const backgroundRect = svg
- .append('rect')
- .attr('class', 'background')
- .attr('width', width)
- .attr('height', height);
+
const g = svg.append('g');
const mapLayer = g.append('g').classed('map-layer', true);
- const textLayer = g
+ const textLayer = svg
.append('g')
- .classed('text-layer', true)
+ .attr('class', 'text-layer')
.attr('transform', `translate(${width / 2}, 45)`);
- const bigText = textLayer.append('text').classed('big-text', true);
+ const bigText = textLayer
+ .append('text')
+ .classed('big-text', true)
+ .style('font-size', '18px');
const resultText = textLayer
.append('text')
.classed('result-text', true)
- .attr('dy', '1em');
-
- let centered;
-
- const clicked = function clicked(d) {
- const hasCenter = d && centered !== d;
- let x;
- let y;
- let k;
- const halfWidth = width / 2;
- const halfHeight = height / 2;
-
- if (hasCenter) {
- const centroid = path.centroid(d);
- [x, y] = centroid;
- k = 4;
- centered = d;
- } else {
- x = halfWidth;
- y = halfHeight;
- k = 1;
- centered = null;
- }
+ .attr('dy', '1em')
+ .style('font-size', '26px');
+
+ // Cross-filter support
+ const getCrossFilterDataMask = source => {
+ const selected = filterState.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
-
`translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
- textLayer
- .style('opacity', 0)
- .attr(
- 'transform',
- `translate(0,0)translate(${x},${hasCenter ? y - 5 : 45})`,
- )
- .transition()
- .duration(750)
- .style('opacity', 1);
- bigText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 6 : 16);
- resultText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 16 : 24);
+ const isSelected = selected.includes(iso);
+ const values = isSelected ? [] : [iso];
+
+ return {
+ dataMask: {
+ extraFormData: {
+ filters: values.length
+ ? [{ col: entity, op: 'IN', val: values }]
+ : [],
+ },
+ filterState: {
+ value: values.length ? values : null,
+ selectedValues: values.length ? values : null,
+ },
+ },
+ isCurrentValueSelected: isSelected,
+ };
};
- backgroundRect.on('click', clicked);
+ // Handle right-click context menu
+ const handleContextMenu = feature => {
+ const pointerEvent = d3.event;
+
+ // Only prevent default if we have a context menu handler
+ if (typeof onContextMenu === 'function') {
+ pointerEvent?.preventDefault();
+ }
+
+ const iso = feature?.properties?.ISO;
+ if (!iso || typeof onContextMenu !== 'function') return;
+
+ const drillVal = iso;
+ const drillToDetailFilters = [
+ { col: entity, op: '==', val: drillVal, formattedVal: drillVal },
+ ];
+ const drillByFilters = [{ col: entity, op: '==', val: drillVal }];
- const selectAndDisplayNameOfRegion = function selectAndDisplayNameOfRegion(
- feature,
- ) {
+ onContextMenu(pointerEvent.clientX, pointerEvent.clientY, {
+ drillToDetail: drillToDetailFilters,
+ crossFilter: getCrossFilterDataMask(feature),
+ drillBy: { filters: drillByFilters, groupbyFieldName: 'entity' },
+ });
+ };
+
+ const selectAndDisplayNameOfRegion = feature => {
let name = '';
- if (feature && feature.properties) {
- if (feature.properties.ID_2) {
- name = feature.properties.NAME_2;
- } else {
- name = feature.properties.NAME_1;
- }
+ if (feature?.properties) {
+ name = feature.properties.NAME_2 || feature.properties.NAME_1 || '';
}
bigText.text(name);
};
- const updateMetrics = function updateMetrics(region) {
- if (region.length > 0) {
- resultText.text(format(region[0].metric));
- }
+ const updateMetrics = regionRows => {
+ if (regionRows?.length > 0) resultText.text(format(regionRows[0].metric));
+ else resultText.text('');
};
- const mouseenter = function mouseenter(d) {
- // Darken color
+ const mouseenter = function (d) {
let c = colorFn(d);
- if (c !== 'none') {
- c = d3.rgb(c).darker().toString();
- }
+ if (c && c !== 'none') c = d3.rgb(c).darker().toString();
d3.select(this).style('fill', c);
selectAndDisplayNameOfRegion(d);
- const result = data.filter(
- region => region.country_id === d.properties.ISO,
- );
+ const result = data.filter(r => r.country_id === d?.properties?.ISO);
updateMetrics(result);
};
- const mouseout = function mouseout() {
- d3.select(this).style('fill', colorFn);
+ const mouseout = function () {
+ d3.select(this).style('fill', d => colorFn(d));
bigText.text('');
resultText.text('');
};
+ // Zoom with panning bounds
+ const zoom = d3.behavior
+ .zoom()
+ .scaleExtent([1, 4])
+ .on('zoom', () => {
+ const { translate, scale } = d3.event; // [tx, ty]
+ let [tx, ty] = translate;
+
+ const scaledW = width * scale;
+ const scaledH = height * scale;
+ const minX = Math.min(0, width - scaledW);
+ const maxX = 0;
+ const minY = Math.min(0, height - scaledH);
+ const maxY = 0;
+
+ // clamp
+ tx = Math.max(Math.min(tx, maxX), minX);
+ ty = Math.max(Math.min(ty, maxY), minY);
+
+ g.attr('transform', `translate(${tx}, ${ty}) scale(${scale})`);
+ zoomStates[chartKey] = { scale, translate: [tx, ty] };
+ });
+
+ d3.select(svg.node()).call(zoom);
+
+ // Restore
+ if (zoomStates[chartKey]) {
+ const { scale, translate } = zoomStates[chartKey];
+ zoom.scale(scale).translate(translate);
+ g.attr(
+ 'transform',
+ `translate(${translate[0]}, ${translate[1]}) scale(${scale})`,
+ );
+ }
+
+ // Visual highlighting for selected regions
+ function highlightSelectedRegion() {
+ const selectedValues = filterState.selectedValues?.length
+ ? filterState.selectedValues
+ : [];
+
+ mapLayer
+ .selectAll('path.region')
+ .style('fill-opacity', d => {
+ const iso = d?.properties?.ISO;
+ return selectedValues.length === 0 || selectedValues.includes(iso)
+ ? 1
+ : 0.3;
+ })
+ .style('stroke', d => {
+ const iso = d?.properties?.ISO;
+ return selectedValues.includes(iso) ? '#222' : null;
+ })
+ .style('stroke-width', d => {
+ const iso = d?.properties?.ISO;
+ return selectedValues.includes(iso) ? '1.5px' : '0.5px';
+ });
+ }
+
+ // Click handler
+ const handleClick = feature => {
+ if (!emitCrossFilters || typeof setDataMask !== 'function') return;
+ const iso = feature?.properties?.ISO;
+ if (!iso) return;
+
+ const baseline = filterState.selectedValues || [];
+ const currently = new Set(baseline);
+ const shift = !!(d3.event && d3.event.shiftKey);
+
+ if (shift) {
+ if (currently.has(iso)) {
+ currently.delete(iso);
+ } else {
+ currently.add(iso);
+ }
+ } else if (currently.size === 1 && currently.has(iso)) {
+ currently.clear();
+ } else {
+ currently.clear();
+ currently.add(iso);
+ }
+
+ const newSelection = Array.from(currently);
+
+ setDataMask({
+ extraFormData: {
+ filters: newSelection.length
+ ? [{ col: entity, op: 'IN', val: newSelection }]
+ : [],
+ },
+ filterState: {
+ value: newSelection.length ? newSelection : null,
+ selectedValues: newSelection.length ? newSelection : null,
+ },
+ });
+
+ highlightSelectedRegion();
Review Comment:
Potential issue: `highlightSelectedRegion()` is called within the
`handleClick` function after calling `setDataMask()`, but the filterState won't
be updated immediately. The highlighting will use stale filterState values.
Consider either passing the new selection to `highlightSelectedRegion()` or
ensuring it reads from the updated state, or rely on the component re-rendering
to call highlighting.
--
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]
