You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "imad-hl (via GitHub)" <gi...@apache.org> on 2025/10/27 16:33:42 UTC
[PR] feat(country-map): add cross-filters support [superset]
imad-hl opened a new pull request, #35859:
URL: https://github.com/apache/superset/pull/35859
### SUMMARY
This PR adds cross-filters support to the legacy CountryMap plugin with some changes to the map interaction UX:
- Clicking on a region now triggers cross-filtering instead of zooming in by default.
- Added multi-select support using **Shift + Click** (toggle regions).
**Note:** While this behavior is not present in other Superset charts, I found it helpful. I welcome guidance from reviewers on whether to keep it or remove it for consistency.
- Selected regions are highlighted, while non-selected ones are faded.
- Zooming and panning are now handled via mouse wheel or trackpad.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
### TESTING INSTRUCTIONS
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [X] Has associated issue: #33772
- [ ] Required feature flags
- [X] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3669293209
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4320528098
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2589018868
##########
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:
Fixed in 9651b63 : passed the new selection directly to `highlightSelectedRegion` to avoid using stale `filterState` values
( Line 311 in CountryMap.js
https://github.com/imad-hl/superset/blob/d818e4a41a0b1abaa92e32a2381ece98c781b00d/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js#L311
)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2682255937
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -156,18 +201,123 @@ function CountryMap(element, props) {
);
};
+ // Mouse move handler to update tooltip position
const mousemove = function mousemove() {
const position = d3.mouse(svg.node());
hoverPopup
.style('top', `${position[1] + 30}px`)
.style('left', `${position[0]}px`);
};
+ // Mouse out handler
const mouseout = function mouseout() {
- d3.select(this).style('fill', colorFn);
+ d3.select(this).style('fill', d => colorFn(d));
hoverPopup.style('display', 'none');
};
+ // Zoom with panning bounds
+ const zoom = d3.behavior
+ .zoom()
+ .scaleExtent([1, 4])
+ .on('zoomstart', () => {
+ svg.style('cursor', 'grabbing');
+ })
+ .on('zoom', () => {
+ const { translate, scale } = d3.event;
+ 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;
+
+ tx = Math.max(Math.min(tx, maxX), minX);
+ ty = Math.max(Math.min(ty, maxY), minY);
Review Comment:
The original code is correct. The panning bounds should use container dimensions (width/height), not map dimensions. This ensures the scaled content stays within the viewport. Using map dimensions causes the map to pan outside the visible container. Tested with Chile map, current implementation works as intended
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3738216765
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3601292175
CodeAnt AI finished reviewing your 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3689408485
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3619117690
Code LGTM, I just want to test before merging. Ephemeral environments are busted at the moment, so it's in my backlog of things to checkout / build / run / test :)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588987386
##########
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:
Fixed in 9651b63 : added a guard check for 'entity' at the start of 'handleClick'. :
##########
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:
Fixed in 9651b63 : added a guard check for 'entity' at the start of 'handleClick'
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2589018868
##########
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:
Fixed in 9651b63 : passed the new selection directly to `highlightSelectedRegion` to avoid using stale `filterState` values
```
const newSelection = Array.from(currently);
...
...
highlightSelectedRegion(newSelection.length ? newSelection : []);
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2890004152
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -63,33 +63,46 @@ interface CountryMapProps {
numberFormat: string;
colorScheme: string;
sliceId: number;
+ onContextMenu?: (clientX: number, clientY: number, data: any) => void;
+ emitCrossFilters?: boolean;
+ setDataMask?: (dataMask: any) => void;
+ filterState?: {
+ selectedValues?: string[];
+ extraFormData?: {
+ filters?: any[];
+ };
+ };
+ entity?: string;
}
const maps: Record<string, GeoData> = {};
+// Store zoom state per chart instance using element as key to enable garbage collection
+const zoomStates = new WeakMap<HTMLElement, { scale: number; translate: [number, number] }>();
function CountryMap(element: HTMLElement, props: CountryMapProps) {
const {
data,
width,
height,
country,
+ entity,
linearColorScheme,
numberFormat,
colorScheme,
sliceId,
+ filterState,
+ emitCrossFilters,
+ onContextMenu,
+ setDataMask,
} = props;
const container = element;
const format = getNumberFormatter(numberFormat);
- const rawExtents = d3Extent(data, v => v.metric);
- const extents: [number, number] =
- rawExtents[0] != null && rawExtents[1] != null
- ? [rawExtents[0], rawExtents[1]]
- : [0, 1];
- const colorSchemeObj = getSequentialSchemeRegistry().get(linearColorScheme);
- const linearColorScale = colorSchemeObj
- ? colorSchemeObj.createLinearScale(extents)
- : () => '#ccc'; // fallback if scheme not found
+
+ const linearColorScale = getSequentialSchemeRegistry()
+ .get(linearColorScheme)
+ .createLinearScale(d3Extent(data, v => v.metric));
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Fix potential runtime error in linearColorScale</b></div>
<div id="fix">
The linearColorScale creation lacks checks for when the color scheme is not found or data is empty, potentially causing runtime errors. The previous code had safeguards; restoring them prevents crashes and maintains fallback behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
const rawExtents = d3Extent(data, v => v.metric);
const extents: [number, number] =
rawExtents[0] != null && rawExtents[1] != null
? [rawExtents[0], rawExtents[1]]
: [0, 1];
const colorSchemeObj = getSequentialSchemeRegistry().get(linearColorScheme);
const linearColorScale = colorSchemeObj
? colorSchemeObj.createLinearScale(extents)
: () => '#ccc'; // fallback if scheme not found
````
</div>
</details>
</div>
<small><i>Code Review Run #84b1f0</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4237750051
@rusackas @villebro
PR ready for review! Just finished my 4th rebase and addressed all Copilot/Bito suggestions.
Would appreciate a review before the next rebase hits. π
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2473983989
##########
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:
I tried using 'zoomend' and throttling to improve performance, but it made the zoom reset after clicks. I decided to keep the original zoom handler and simply update the WeakMap when the transform actually changes, which keeps zoom smooth and avoids memory issues.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2589041512
##########
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:
Fixed in 9651b63 : replaced the hardcoded string 'entity' with the actual `entity` prop
(Line 172 in CountryMap.js :
https://github.com/imad-hl/superset/blob/d818e4a41a0b1abaa92e32a2381ece98c781b00d/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js#L172
)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3696629149
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588993371
##########
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:
Fixed in 9651b63 : replaced the global `zoomStates` object with a `WeakMap` to allow garbage collection and prevent memory leaks.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4773747372
<!-- translation-regression-bot -->
## β οΈ Translation Regression Detected
A source change in this PR renamed or reworded strings, invalidating existing translations (they are now `#, fuzzy`) in `fi`, `it`, `ko`, `th`. Please resolve the affected `.po` files before merging.
_Note: intentionally **deleting** a translatable string is not a regression and is not flagged here β only translations invalidated by a renamed/reworded source string are._
| Language | Fuzzy before | Fuzzy after | New |
|----------|-------------:|------------:|----:|
| `fi` | 4815 | 4817 | +2 |
| `it` | 1665 | 1667 | +2 |
| `ko` | 1519 | 1521 | +2 |
| `th` | 4815 | 4817 | +2 |
### How to fix
**1. Install dependencies** (if not already set up):
```bash
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext
```
**2. Re-extract strings and sync `.po` files:**
```bash
./scripts/translations/babel_update.sh
```
This rewrites `superset/translations/messages.pot` from the current source files and merges the changes into every `.po` file. Strings whose `msgid` changed will be marked `#, fuzzy`.
**3. Resolve the fuzzy entries** in the affected language files (`fi`, `it`, `ko`, `th`):
```bash
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.po
```
For each fuzzy entry, either rewrite the `msgstr` to match the new string and remove the `#, fuzzy` line, or clear the `msgstr` to `""` if you cannot provide a translation.
**4. Commit your changes to the `.po` files.**
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4774844317
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #35859:
URL: https://github.com/apache/superset/pull/35859
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3544150635
Looks like there are some typescript issues to fix. You might fix some by running `npm run plugins:build` on the frontend.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588979308
##########
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:
Fixed in 9651b63 : changed the default color for regions without data to a more neutral light gray '#d9d9d9'
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3628327554
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20036613186) for [d818e4a](https://github.com/apache/superset/commit/d818e4a)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3628377896
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20036613186) for [d818e4a](https://github.com/apache/superset/commit/d818e4a)
β’ **Environment:** http://52.35.39.226:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3629495950
Eek... some conflicts have arisen due to another recently merged PR. Could you take a look at rebasing? Hope I didn't cause too much trouble.
Otherwise, I think the shift-click thing is a little... unusual... but I don't mind it all that much. It's hard to discover, so a docs entry couldn't hurt, OR adapting this to other cross-filter-emitting charts like Pie Charts or Bar Charts. That'd be cool, unless there's a better, more visibly obvious pattern here (@kasiazjc might have ideas).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3647021245
> Eek... some conflicts have arisen due to another recently merged PR. Could you take a look at rebasing? Hope I didn't cause too much trouble.
>
> Otherwise, I think the shift-click thing is a little... unusual... but I don't mind it all that much. It's hard to discover, so a docs entry couldn't hurt, OR adapting this to other cross-filter-emitting charts like Pie Charts or Bar Charts. That'd be cool, unless there's a better, more visibly obvious pattern here (@kasiazjc might have ideas).
Hi! I think I got the conflicts sorted out. The recently merged PR added a nice tooltip feature on hover, which is now integrated.
About the shift-click multiselect : I agree it's not very discoverable. I've removed it and simplified the behavior to match what other cross-filter charts do (Pie, Bar, etc.).
I do think shift-multiselect could be a good addition if implemented consistently across all cross-filter charts, but for this PR I'll stick with removing it and keeping things aligned with the existing patterns.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4318564422
Hey @imad-hl - I didn't want to ask you to do any more work, since I've been far too slow to get back to this (again).
I opened a PR targeting your branch, making some tweaks/adjustments that I was going to request:
https://github.com/imad-hl/superset/pull/4
If you like it, merge it into your branch, and (I think) it should all get picked up here so we can merge this :D
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r3455865773
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -175,34 +239,134 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
.classed('popup-at-bottom', y > (svgHeight * 2) / 3);
};
- const mouseenter = function mouseenter(this: SVGPathElement, d: GeoFeature) {
+ const mouseenter = function mouseenter(
+ this: SVGPathElement,
+ d: GeoFeature,
+ ): void {
// Darken color
let c: string = colorFn(d);
- if (c !== 'none') {
+ if (c) {
c = d3.rgb(c).darker().toString();
}
d3.select(this).style('fill', c);
- // Display information popup
- const result = data.filter(
- region => region.country_id === d.properties.ISO,
- );
- hoverPopup.style('display', 'block').html(
- `<div><strong>${getNameOfRegion(d)}</strong><br>${result.length > 0 ? formatter(result[0].metric) : ''}</div>`,
- );
+ // Display information popup
+ const result = data.filter(r => r.country_id === d?.properties?.ISO);
+ hoverPopup
+ .style('display', 'block')
+ .html(
+ `<div><strong>${getNameOfRegion(d)}</strong><br>${result.length > 0 ? formatter(result[0].metric) : ''}</div>`,
+ );
Review Comment:
Good catch, pushed a fix for this (451b9dbfc3) - escaped the region name and the value through the `escapeHtml` helper that's already used elsewhere in this file. The country geojsons are bundled assets so the real-world risk is low, but no reason not to escape it.
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -112,46 +146,76 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
+
+ // Only set grab cursor if not in edit mode
+ if (!isEditMode) {
+ svg.style('cursor', 'grab');
+ }
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);
+ // Add hover popup for tooltip
const hoverPopup = div.append('div').attr('class', 'hover-popup');
- let centered: GeoFeature | null;
-
- const clicked = function clicked(d: GeoFeature) {
- const hasCenter = d && centered !== d;
- let x: number;
- let y: number;
- let k: number;
- 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;
- }
+ // Track mouse position to distinguish clicks from drags
+ let mousedownPos: { x: number; y: number } | null = null;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
- `translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
+ // Cross-filter support
+ const getCrossFilterDataMask = (
+ source: GeoFeature,
+ ): { dataMask: DataMask; isCurrentValueSelected: boolean } | undefined => {
+ if (!entity) return undefined;
+
+ const selected = filterState?.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
+
+ const isSelected = selected.includes(iso);
+ const values = isSelected ? [] : [iso];
Review Comment:
I think this one is working as intended... there's no Shift+Click/multi-select code in here (the author noted in the description they'd drop it for consistency with the other charts), so this is plain single-select cross-filtering: clicking a region sets the filter, clicking it again clears it. The collapse to one-or-none is the desired behavior here rather than a regression.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4259122779
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3689256115
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588957216
##########
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:
Fixed in 9651b63 : added a guard check for 'entity' at the beginning of 'getCrossFilterDataMask'
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588993371
##########
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:
Fixed in 9651b63 : replaced the global `zoomStates` object with a `WeakMap` to allow garbage collection and prevent memory leaks
( Line 54 in CountryMap.js :
https://github.com/imad-hl/superset/blob/d818e4a41a0b1abaa92e32a2381ece98c781b00d/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js#L54
)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2473983989
##########
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:
I tried using 'zoomend' and throttling to improve performance, but it made the zoom reset after clicks.
so in 9651b63 : I decided to keep the original zoom handler and simply update the WeakMap when the transform actually changes, which keeps zoom smooth and avoids memory issues.
( See Lines 223 -> 234 in CountryMap.js
https://github.com/imad-hl/superset/blob/d818e4a41a0b1abaa92e32a2381ece98c781b00d/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js#L223
https://github.com/imad-hl/superset/blob/d818e4a41a0b1abaa92e32a2381ece98c781b00d/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js#L234
)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2589037276
##########
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:
Fixed in 9651b63 : added optional chaining for 'filterState' in 'getCrossFilterDataMask' to safely access 'selectedValues'
( Line 128 in CountryMap.js :
https://github.com/imad-hl/superset/blob/d818e4a41a0b1abaa92e32a2381ece98c781b00d/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js#L128
)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r3465274080
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -112,46 +146,76 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
+
+ // Only set grab cursor if not in edit mode
+ if (!isEditMode) {
+ svg.style('cursor', 'grab');
+ }
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);
+ // Add hover popup for tooltip
const hoverPopup = div.append('div').attr('class', 'hover-popup');
- let centered: GeoFeature | null;
-
- const clicked = function clicked(d: GeoFeature) {
- const hasCenter = d && centered !== d;
- let x: number;
- let y: number;
- let k: number;
- 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;
- }
+ // Track mouse position to distinguish clicks from drags
+ let mousedownPos: { x: number; y: number } | null = null;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
- `translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
+ // Cross-filter support
+ const getCrossFilterDataMask = (
+ source: GeoFeature,
+ ): { dataMask: DataMask; isCurrentValueSelected: boolean } | undefined => {
+ if (!entity) return undefined;
+
+ const selected = filterState?.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
+
+ 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: GeoFeature): void => {
+ const pointerEvent = d3.event;
+
+ if (typeof onContextMenu === 'function') {
+ pointerEvent?.preventDefault();
+ }
+
+ const iso = feature?.properties?.ISO;
+ if (!iso || typeof onContextMenu !== 'function' || !entity) return;
+
+ const drillVal = iso;
+ const drillToDetailFilters = [
+ { col: entity, op: '==', val: drillVal, formattedVal: drillVal },
+ ];
+ const drillByFilters = [{ col: entity, op: '==', val: drillVal }];
+
+ onContextMenu(pointerEvent.clientX, pointerEvent.clientY, {
+ drillToDetail: drillToDetailFilters,
+ crossFilter: getCrossFilterDataMask(feature),
+ drillBy: { filters: drillByFilters, groupbyFieldName: entity },
Review Comment:
Investigating this... I'll patch it if it's legit. Would love to see this finally merge :)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r3465109132
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -112,46 +146,76 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
+
+ // Only set grab cursor if not in edit mode
+ if (!isEditMode) {
+ svg.style('cursor', 'grab');
+ }
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);
+ // Add hover popup for tooltip
const hoverPopup = div.append('div').attr('class', 'hover-popup');
- let centered: GeoFeature | null;
-
- const clicked = function clicked(d: GeoFeature) {
- const hasCenter = d && centered !== d;
- let x: number;
- let y: number;
- let k: number;
- 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;
- }
+ // Track mouse position to distinguish clicks from drags
+ let mousedownPos: { x: number; y: number } | null = null;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
- `translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
+ // Cross-filter support
+ const getCrossFilterDataMask = (
+ source: GeoFeature,
+ ): { dataMask: DataMask; isCurrentValueSelected: boolean } | undefined => {
+ if (!entity) return undefined;
+
+ const selected = filterState?.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
+
+ 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: GeoFeature): void => {
+ const pointerEvent = d3.event;
+
+ if (typeof onContextMenu === 'function') {
+ pointerEvent?.preventDefault();
+ }
+
+ const iso = feature?.properties?.ISO;
+ if (!iso || typeof onContextMenu !== 'function' || !entity) return;
+
+ const drillVal = iso;
+ const drillToDetailFilters = [
+ { col: entity, op: '==', val: drillVal, formattedVal: drillVal },
+ ];
+ const drillByFilters = [{ col: entity, op: '==', val: drillVal }];
+
+ onContextMenu(pointerEvent.clientX, pointerEvent.clientY, {
+ drillToDetail: drillToDetailFilters,
+ crossFilter: getCrossFilterDataMask(feature),
+ drillBy: { filters: drillByFilters, groupbyFieldName: entity },
Review Comment:
**Suggestion:** `groupbyFieldName` in drill-by payload must be the form-data control key, not the selected column value. Passing `entity` here sends values like `"country_code"` instead of `"entity"`, so drill-by cannot map the selection back to the chart control and the action can fail or apply incorrectly. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Country Map drill-by fails to update groupby control.
- β οΈ Drill-by submenu appears but produces incorrect drilled charts.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Configure a Country Map chart in Explore using the "ISO 3166-2 Codes" control named
`entity`
(`superset-frontend/plugins/legacy-plugin-chart-country-map/src/controlPanel.ts:29-49`),
e.g. select a column `country_code`.
2. When the chart renders, `transformProps` passes `formData.entity` into the `entity`
prop along with hooks
(`superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.ts:21-41,62-79`),
so inside `CountryMap` the `entity` variable holds the selected column name like
`"country_code"`.
3. Right-click a region on the map; the D3 handler `handleContextMenu` in `CountryMap.ts`
builds `drillByFilters` and calls `onContextMenu(pointerEvent.clientX,
pointerEvent.clientY, { drillToDetail, crossFilter, drillBy: { filters: drillByFilters,
groupbyFieldName: entity } })` (`CountryMap.ts:196-218`), so `groupbyFieldName` is set to
the column name (`"country_code"`) instead of the form control key (`"entity"`).
4. The Chart layer receives these filters in `ChartRenderer.handleOnContextMenu`
(`src/components/Chart/ChartRenderer.tsx:320-337`), forwards them to `ChartContextMenu`,
which passes `filters.drillBy` into `DrillByModal` via `DrillBySubmenu`
(`ChartContextMenu.tsx:340-41). In `DrillByModal.getFormDataChangesFromConfigs`,
`config.groupbyFieldName` is used as a key into `formData` (`DrillByModal.tsx:220-36`);
because `groupbyFieldName` is `"country_code"` (a column label) and not `"entity"` (the
real field name), `formData[config.groupbyFieldName]` is undefined and the updated
formData gets a new `"country_code"` property instead of updating `formData.entity`. This
breaks drill-by for Country Map (the new chart does not drill on the selected dimension),
whereas other charts correctly pass control keys like `'groupby'`
(`plugin-chart-echarts/src/utils/eventHandlers.ts:87-92`,
`Timeseries/EchartsTimeseries.tsx:320-15`, `Treemap/EchartsTreemap.tsx:140-13`,
`Graph/EchartsGraph.tsx:151-17`).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=03cf95bca42146509f1c093bf8678320&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=03cf95bca42146509f1c093bf8678320&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts
**Line:** 216:216
**Comment:**
*Api Mismatch: `groupbyFieldName` in drill-by payload must be the form-data control key, not the selected column value. Passing `entity` here sends values like `"country_code"` instead of `"entity"`, so drill-by cannot map the selection back to the chart control and the action can fail or apply incorrectly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35859&comment_hash=dc63b8aaad0008755145ba96065b1e493878def0a14663fa75dda9d1db8ee00e&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35859&comment_hash=dc63b8aaad0008755145ba96065b1e493878def0a14663fa75dda9d1db8ee00e&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2890695826
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -167,15 +220,13 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
const mouseenter = function mouseenter(this: SVGPathElement, d: GeoFeature) {
// Darken color
let c: string = colorFn(d);
- if (c !== 'none') {
+ if (c && c !== 'none') {
c = d3.rgb(c).darker().toString();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Obsolete condition in hover darkening</b></div>
<div id="fix">
The check for 'none' in the mouseenter handler is obsolete since colorFn now returns '#d9d9d9' instead of 'none' for missing data. This could cause the fallback gray to be incorrectly darkened on hover, altering the visual behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
let c: string = colorFn(d);
if (c && c !== '#d9d9d9') {
c = d3.rgb(c).darker().toString();
````
</div>
</details>
</div>
<small><i>Code Review Run #93546d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2682014343
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -156,18 +201,123 @@ function CountryMap(element, props) {
);
};
+ // Mouse move handler to update tooltip position
const mousemove = function mousemove() {
const position = d3.mouse(svg.node());
hoverPopup
.style('top', `${position[1] + 30}px`)
.style('left', `${position[0]}px`);
};
+ // Mouse out handler
const mouseout = function mouseout() {
- d3.select(this).style('fill', colorFn);
+ d3.select(this).style('fill', d => colorFn(d));
hoverPopup.style('display', 'none');
};
+ // Zoom with panning bounds
+ const zoom = d3.behavior
+ .zoom()
+ .scaleExtent([1, 4])
+ .on('zoomstart', () => {
+ svg.style('cursor', 'grabbing');
+ })
+ .on('zoom', () => {
+ const { translate, scale } = d3.event;
+ 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;
+
+ tx = Math.max(Math.min(tx, maxX), minX);
+ ty = Math.max(Math.min(ty, maxY), minY);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect zoom panning bounds</b></div>
<div id="fix">
The zoom panning bounds use container width/height * scale, but should use the map's actual pixel dimensions * scale to correctly constrain panning. For maps not filling the container (e.g., tall maps), this allows over-panning or restricts valid pans. Store bounds in zoomStates and retrieve in the zoom handler.
</div>
</div>
<small><i>Code Review Run #95794b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r3465308766
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -112,46 +146,76 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
+
+ // Only set grab cursor if not in edit mode
+ if (!isEditMode) {
+ svg.style('cursor', 'grab');
+ }
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);
+ // Add hover popup for tooltip
const hoverPopup = div.append('div').attr('class', 'hover-popup');
- let centered: GeoFeature | null;
-
- const clicked = function clicked(d: GeoFeature) {
- const hasCenter = d && centered !== d;
- let x: number;
- let y: number;
- let k: number;
- 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;
- }
+ // Track mouse position to distinguish clicks from drags
+ let mousedownPos: { x: number; y: number } | null = null;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
- `translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
+ // Cross-filter support
+ const getCrossFilterDataMask = (
+ source: GeoFeature,
+ ): { dataMask: DataMask; isCurrentValueSelected: boolean } | undefined => {
+ if (!entity) return undefined;
+
+ const selected = filterState?.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
+
+ 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: GeoFeature): void => {
+ const pointerEvent = d3.event;
+
+ if (typeof onContextMenu === 'function') {
+ pointerEvent?.preventDefault();
+ }
+
+ const iso = feature?.properties?.ISO;
+ if (!iso || typeof onContextMenu !== 'function' || !entity) return;
+
+ const drillVal = iso;
+ const drillToDetailFilters = [
+ { col: entity, op: '==', val: drillVal, formattedVal: drillVal },
+ ];
+ const drillByFilters = [{ col: entity, op: '==', val: drillVal }];
+
+ onContextMenu(pointerEvent.clientX, pointerEvent.clientY, {
+ drillToDetail: drillToDetailFilters,
+ crossFilter: getCrossFilterDataMask(feature),
+ drillBy: { filters: drillByFilters, groupbyFieldName: entity },
Review Comment:
Good catch, this one's legit. `groupbyFieldName` was getting the selected column value instead of the literal `'entity'` control key, so `DrillByModal` couldn't map it back. Fixed it to match the world-map sibling chart, and added test coverage for the drill-by/cross-filter paths while I was in here. Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588968360
##########
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:
Fixed in 9651b63 : added optional chaining for 'filterState' to safely access 'selectedValues'
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3612060945
> Re-running CI, let's see how it goes! π€
>
> That said, there are still a lot of Copilot suggestions, if you don't mind addressing them (leaving a comment if they're irrelevant/wrong, or addressing them if they're on the right track).
I've addressed the Copilot suggestions in commit 9651b63 hence why they show as "outdated". but I'll go through each one and add comments explaining the implemented solution
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588979308
##########
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:
Fixed in 9651b63 : changed the default color for regions without data to a more neutral light gray '#d9d9d9'
( Line 92 in CountryMap.js :
https://github.com/imad-hl/superset/blob/d818e4a41a0b1abaa92e32a2381ece98c781b00d/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js#L92
)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3452261984
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4237678584
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4318433532
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/35859?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `36.97479%` with `75 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.51%. Comparing base ([`0b419a0`](https://app.codecov.io/gh/apache/superset/commit/0b419a07f54f3de1b0bb89a41c9b5b047f928f79?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4815976`](https://app.codecov.io/gh/apache/superset/commit/481597616044ce4d92e1c725b4b8b635107fae0d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 138 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/35859?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../legacy-plugin-chart-country-map/src/CountryMap.ts](https://app.codecov.io/gh/apache/superset/pull/35859?src=pr&el=tree&filepath=superset-frontend%2Fplugins%2Flegacy-plugin-chart-country-map%2Fsrc%2FCountryMap.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNvdW50cnktbWFwL3NyYy9Db3VudHJ5TWFwLnRz) | 37.93% | [72 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/35859?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...acy-plugin-chart-country-map/src/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/35859?src=pr&el=tree&filepath=superset-frontend%2Fplugins%2Flegacy-plugin-chart-country-map%2Fsrc%2FtransformProps.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNvdW50cnktbWFwL3NyYy90cmFuc2Zvcm1Qcm9wcy50cw==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/35859?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
:x: Your project check has failed because the head coverage (99.79%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #35859 +/- ##
==========================================
- Coverage 64.54% 64.51% -0.03%
==========================================
Files 2557 2557
Lines 132985 133076 +91
Branches 30890 30922 +32
==========================================
+ Hits 85829 85860 +31
- Misses 45666 45726 +60
Partials 1490 1490
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/35859/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/35859/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.29% <36.97%> (-0.04%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/35859?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r3092455034
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -187,19 +245,123 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
);
};
- const mousemove = function mousemove() {
+ // Mouse move handler to update tooltip position
+ const mousemove = function mousemove(): void {
const position = d3.mouse(svg.node());
hoverPopup
.style('top', `${position[1] + 30}px`)
.style('left', `${position[0]}px`);
};
- const mouseout = function mouseout(this: SVGPathElement) {
- d3.select(this).style('fill', colorFn);
+ const mouseout = function mouseout(this: SVGPathElement): void {
+ d3.select(this).style('fill', (d: GeoFeature) => colorFn(d));
hoverPopup.style('display', 'none');
};
- function drawMap(mapData: GeoData) {
+ // Only enable zoom if not in edit mode
+ if (!isEditMode) {
+ // Zoom with panning bounds
+ const zoom = d3.behavior
+ .zoom()
+ .scaleExtent([1, 4])
+ .on('zoomstart', () => {
+ svg.style('cursor', 'grabbing');
+ })
+ .on('zoom', () => {
+ const { translate, scale } = d3.event;
+ 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;
+
+ tx = Math.max(Math.min(tx, maxX), minX);
+ ty = Math.max(Math.min(ty, maxY), minY);
+
+ g.attr('transform', `translate(${tx}, ${ty}) scale(${scale})`);
+ const prev = zoomStates.get(element);
+ const changed =
+ !prev ||
+ prev.scale !== scale ||
+ prev.translate[0] !== tx ||
+ prev.translate[1] !== ty;
+ if (changed) {
+ zoomStates.set(element, { scale, translate: [tx, ty] });
+ }
+ })
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Zoom panning constraint bug</b></div>
<div id="fix">
After clamping `tx` and `ty` in the zoom event handler, call `zoom.translate([tx, ty])` so the D3 v3 zoom behaviorβs internal translate state is updated.
</div>
</div>
<small><i>Code Review Run #81de52</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -112,46 +139,76 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
+
+ // Only set grab cursor if not in edit mode
+ if (!isEditMode) {
+ svg.style('cursor', 'grab');
+ }
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);
+ // Add hover popup for tooltip
const hoverPopup = div.append('div').attr('class', 'hover-popup');
- let centered: GeoFeature | null;
-
- const clicked = function clicked(d: GeoFeature) {
- const hasCenter = d && centered !== d;
- let x: number;
- let y: number;
- let k: number;
- 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;
- }
+ // Track mouse position to distinguish clicks from drags
+ let mousedownPos: { x: number; y: number } | null = null;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
- `translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
+ // Cross-filter support
+ const getCrossFilterDataMask = (
+ source: GeoFeature,
+ ): { dataMask: any; isCurrentValueSelected: boolean } | undefined => {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>TypeScript any type violation</b></div>
<div id="fix">
The return type uses 'any' for dataMask, violating the no-any-types rule in dev-standard.mdc. Define a proper type based on the object structure to improve type safety and maintainability.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -160,0 +160,8 @@
+type DataMask = {
+ extraFormData: {
+ filters: { col: string; op: 'IN'; val: string[] }[];
+ };
+ filterState: {
+ value: string[] | null;
+ selectedValues: string[] | null;
+ };
+};
+// Cross-filter support
```
</div>
</details>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a href="https://github.com/apache/superset/blob/48c76b0/.cursor/rules/dev-standard.mdc#L16">dev-standard.mdc:16</a>
</li>
</ul>
</details>
<small><i>Code Review Run #81de52</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4004980424
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4008423876
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588957216
##########
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:
Fixed in 9651b63 : added a guard check for 'entity' at the beginning of 'getCrossFilterDataMask'
( Line :
https://github.com/imad-hl/superset/blob/d818e4a41a0b1abaa92e32a2381ece98c781b00d/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js#L126
)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588968360
##########
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:
Fixed in 9651b63 : added optional chaining for 'filterState' to safely access 'selectedValues' in 'handleClick'
##########
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:
Fixed in 9651b63 : added optional chaining for 'filterState' in 'getCrossFilterDataMask' to safely access 'selectedValues'
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3737974201
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | c34262c574ae8ff4b266b4516214f2774a8114af |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6964d3139c024d0008210572 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-35859--superset-docs-preview.netlify.app](https://deploy-preview-35859--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-35859--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4786383335
Looks like this is conflicting *again*. Let me rebase your branch to clear it and get my tweaks PR folded in so we can finally merge this.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3696781223
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588957216
##########
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:
Fixed in 9651b63 β added a guard check for 'entity' at the beginning of 'getCrossFilterDataMask'
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588968360
##########
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:
Fixed in 9651b63 : added optional chaining for 'filterState' to safely access 'selectedValues' in 'handleClick'
( Line :
https://github.com/imad-hl/superset/blob/d818e4a41a0b1abaa92e32a2381ece98c781b00d/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js#L279
)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2473983989
##########
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:
I tried using 'zoomend' and throttling to improve performance, but it made the zoom reset after clicks.
so in 9651b63 : I decided to keep the original zoom handler and simply update the WeakMap when the transform actually changes, which keeps zoom smooth and avoids memory issues.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2588987386
##########
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:
Fixed in 9651b63 : added a guard check for 'entity' at the start of 'handleClick'
( Line 272 in CountryMap.js :
https://github.com/imad-hl/superset/blob/d818e4a41a0b1abaa92e32a2381ece98c781b00d/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js#L272
)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r3074227045
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -112,46 +139,76 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
+
+ // Only set grab cursor if not in edit mode
+ if (!isEditMode) {
+ svg.style('cursor', 'grab');
+ }
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);
+ // Add hover popup for tooltip
const hoverPopup = div.append('div').attr('class', 'hover-popup');
- let centered: GeoFeature | null;
-
- const clicked = function clicked(d: GeoFeature) {
- const hasCenter = d && centered !== d;
- let x: number;
- let y: number;
- let k: number;
- 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;
- }
+ // Track mouse position to distinguish clicks from drags
+ let mousedownPos: { x: number; y: number } | null = null;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
- `translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
+ // Cross-filter support
+ const getCrossFilterDataMask = (
+ source: GeoFeature,
+ ): { dataMask: any; isCurrentValueSelected: boolean } | undefined => {
+ if (!entity) return undefined;
+
+ const selected = filterState?.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
+
+ 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,
+ },
+ },
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Code Duplication</b></div>
<div id="fix">
The dataMask creation logic is duplicated between `getCrossFilterDataMask` (lines 174-184) and `handleClick` (lines 349-359). This creates maintenance risk if the logic needs to change. Extract a shared helper function to ensure consistency.
</div>
</div>
<small><i>Code Review Run #fdf57c</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4238173845
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2682263912
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -156,18 +201,123 @@ function CountryMap(element, props) {
);
};
+ // Mouse move handler to update tooltip position
const mousemove = function mousemove() {
const position = d3.mouse(svg.node());
hoverPopup
.style('top', `${position[1] + 30}px`)
.style('left', `${position[0]}px`);
};
+ // Mouse out handler
const mouseout = function mouseout() {
- d3.select(this).style('fill', colorFn);
+ d3.select(this).style('fill', d => colorFn(d));
hoverPopup.style('display', 'none');
};
+ // Zoom with panning bounds
+ const zoom = d3.behavior
+ .zoom()
+ .scaleExtent([1, 4])
+ .on('zoomstart', () => {
+ svg.style('cursor', 'grabbing');
+ })
+ .on('zoom', () => {
+ const { translate, scale } = d3.event;
+ 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;
+
+ tx = Math.max(Math.min(tx, maxX), minX);
+ ty = Math.max(Math.min(ty, maxY), minY);
Review Comment:
<!-- Bito Reply -->
The PR's zoom implementation uses container width and height for panning bounds, ensuring the scaled map stays within the viewport. This aligns with your explanation.
**superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js**
```
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;
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3738032104
> re-running CI π€
CI passed earlier, but another PR (#36929) was merged and caused a small conflict in **_index.js_**.
The conflict has now been resolved
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2682255937
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -156,18 +201,123 @@ function CountryMap(element, props) {
);
};
+ // Mouse move handler to update tooltip position
const mousemove = function mousemove() {
const position = d3.mouse(svg.node());
hoverPopup
.style('top', `${position[1] + 30}px`)
.style('left', `${position[0]}px`);
};
+ // Mouse out handler
const mouseout = function mouseout() {
- d3.select(this).style('fill', colorFn);
+ d3.select(this).style('fill', d => colorFn(d));
hoverPopup.style('display', 'none');
};
+ // Zoom with panning bounds
+ const zoom = d3.behavior
+ .zoom()
+ .scaleExtent([1, 4])
+ .on('zoomstart', () => {
+ svg.style('cursor', 'grabbing');
+ })
+ .on('zoom', () => {
+ const { translate, scale } = d3.event;
+ 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;
+
+ tx = Math.max(Math.min(tx, maxX), minX);
+ ty = Math.max(Math.min(ty, maxY), minY);
Review Comment:
Applied the hooks default fix !
However, the zoom panning bounds suggestion is incorrect. The original code properly uses container dimensions to keep scaled content within the viewport. Using map dimensions causes the map to pan outside the visible area. Tested with Chile map and confirmed current implementation works correctly
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3619129905
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19980650228) for [d818e4a](https://github.com/apache/superset/commit/d818e4a)
β’ **Environment:** http://34.222.53.98:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3604180122
Re-running CI, let's see how it goes! π€
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3646962055
CodeAnt AI is running Incremental review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2660842340
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -179,34 +322,37 @@ function CountryMap(element, props) {
.translate([width / 2, height / 2]);
path.projection(projection);
- // Compute scale that fits container.
const bounds = path.bounds(mapData);
const hscale = (scale * width) / (bounds[1][0] - bounds[0][0]);
const vscale = (scale * height) / (bounds[1][1] - bounds[0][1]);
- const newScale = hscale < vscale ? hscale : vscale;
+ const newScale = Math.min(hscale, vscale);
- // Compute bounds and offset using the updated scale.
projection.scale(newScale);
const newBounds = path.bounds(mapData);
projection.translate([
width - (newBounds[0][0] + newBounds[1][0]) / 2,
height - (newBounds[0][1] + newBounds[1][1]) / 2,
]);
- // Draw each province as a path
- mapLayer
- .selectAll('path')
- .data(features)
+ const sel = mapLayer.selectAll('path.region').data(features);
+
+ sel
.enter()
.append('path')
- .attr('d', path)
.attr('class', 'region')
.attr('vector-effect', 'non-scaling-stroke')
+ .attr('d', path)
.style('fill', colorFn)
.on('mouseenter', mouseenter)
.on('mousemove', mousemove)
.on('mouseout', mouseout)
- .on('click', clicked);
+ .on('contextmenu', handleContextMenu)
+ .on('click', handleClick );
+
+ mapLayer.selectAll('path.region').attr('d', path).style('fill', colorFn);
+ sel.exit().remove();
+
+ highlightSelectedRegion();
Review Comment:
<!-- Bito Reply -->
This question isnβt related to the pull request. I can only help with questions about the PRβs code or comments.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4773528452
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4787977461
**Bito Automatic Review Skipped β PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4237518949
@rusackas @villebro
PR ready for review! Just finished my 4th rebase and addressed all Copilot/Bito suggestions.
Would appreciate a review before the next rebase hits. π
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4320420185
> Hey @imad-hl - I didn't want to ask you to do any more work, since I've been far too slow to get back to this (again).
>
> I opened a PR targeting your branch, making some tweaks/adjustments that I was going to request: [imad-hl#4](https://github.com/imad-hl/superset/pull/4)
>
> If you like it, merge it into your branch, and (I think) it should all get picked up here so we can merge this :D
No worries at all π thanks for taking the time to make those improvements.
Iβve reviewed the changes and just merged your PR. Looks good to me π
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4005791995
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3619118688
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19980650228) for [d818e4a](https://github.com/apache/superset/commit/d818e4a)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3737959475
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3738804206
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3452261538
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](43cfae0c-ba09-4b93-8c5d-7377a193093d)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3547032992
> Looks like there are some typescript issues to fix. You might fix some by running `npm run plugins:build` on the frontend.
Hi @rusackas
Thanks for reviewing! I ran `npm run plugins:build` locally and the Country Map plugin seems to compile successfully.
apart from some babel warnings :
```
( @superset-ui/legacy-plugin-chart-country-map: `isModuleDeclaration` has been deprecated, please migrate to `isImportOrExportDeclaration`
@superset-ui/legacy-plugin-chart-country-map: at isModuleDeclaration (/app/superset-frontend/node_modules/@babel/types/lib/validators/generated/index.js:2793:35)
@superset-ui/legacy-plugin-chart-country-map: at PluginPass.Program (/app/superset-frontend/node_modules/babel-plugin-lodash/lib/index.js:102:44))
```
The type script errors seems to come from other plugins, I rebased my branch on the latest master, hope it helps
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3601279562
CodeAnt AI is reviewing your 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2645416175
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -179,34 +322,37 @@ function CountryMap(element, props) {
.translate([width / 2, height / 2]);
path.projection(projection);
- // Compute scale that fits container.
const bounds = path.bounds(mapData);
const hscale = (scale * width) / (bounds[1][0] - bounds[0][0]);
const vscale = (scale * height) / (bounds[1][1] - bounds[0][1]);
- const newScale = hscale < vscale ? hscale : vscale;
+ const newScale = Math.min(hscale, vscale);
- // Compute bounds and offset using the updated scale.
projection.scale(newScale);
const newBounds = path.bounds(mapData);
projection.translate([
width - (newBounds[0][0] + newBounds[1][0]) / 2,
height - (newBounds[0][1] + newBounds[1][1]) / 2,
]);
- // Draw each province as a path
- mapLayer
- .selectAll('path')
- .data(features)
+ const sel = mapLayer.selectAll('path.region').data(features);
+
+ sel
.enter()
.append('path')
- .attr('d', path)
.attr('class', 'region')
.attr('vector-effect', 'non-scaling-stroke')
+ .attr('d', path)
.style('fill', colorFn)
.on('mouseenter', mouseenter)
.on('mousemove', mousemove)
.on('mouseout', mouseout)
- .on('click', clicked);
+ .on('contextmenu', handleContextMenu)
+ .on('click', handleClick );
+
+ mapLayer.selectAll('path.region').attr('d', path).style('fill', colorFn);
+ sel.exit().remove();
+
+ highlightSelectedRegion();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>D3 Update Selection Missing Events</b></div>
<div id="fix">
Event handlers are set only on enter(), so existing map regions lose mouse/click interactivity when features update. Add .on() calls to the update line.
</div>
</div>
<small><i>Code Review Run #9a6202</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "imad-hl (via GitHub)" <gi...@apache.org>.
imad-hl commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r2660840219
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -179,34 +322,37 @@ function CountryMap(element, props) {
.translate([width / 2, height / 2]);
path.projection(projection);
- // Compute scale that fits container.
const bounds = path.bounds(mapData);
const hscale = (scale * width) / (bounds[1][0] - bounds[0][0]);
const vscale = (scale * height) / (bounds[1][1] - bounds[0][1]);
- const newScale = hscale < vscale ? hscale : vscale;
+ const newScale = Math.min(hscale, vscale);
- // Compute bounds and offset using the updated scale.
projection.scale(newScale);
const newBounds = path.bounds(mapData);
projection.translate([
width - (newBounds[0][0] + newBounds[1][0]) / 2,
height - (newBounds[0][1] + newBounds[1][1]) / 2,
]);
- // Draw each province as a path
- mapLayer
- .selectAll('path')
- .data(features)
+ const sel = mapLayer.selectAll('path.region').data(features);
+
+ sel
.enter()
.append('path')
- .attr('d', path)
.attr('class', 'region')
.attr('vector-effect', 'non-scaling-stroke')
+ .attr('d', path)
.style('fill', colorFn)
.on('mouseenter', mouseenter)
.on('mousemove', mousemove)
.on('mouseout', mouseout)
- .on('click', clicked);
+ .on('contextmenu', handleContextMenu)
+ .on('click', handleClick );
+
+ mapLayer.selectAll('path.region').attr('d', path).style('fill', colorFn);
+ sel.exit().remove();
+
+ highlightSelectedRegion();
Review Comment:
Fixed in 6cf5f86 :
- event handlers now apply to all regions (new + existing) and
- color fallback is now consistent using `#d9d9d9` instead of `'none'`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-3711370781
re-running CI π€
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r3454821068
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -175,34 +239,134 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
.classed('popup-at-bottom', y > (svgHeight * 2) / 3);
};
- const mouseenter = function mouseenter(this: SVGPathElement, d: GeoFeature) {
+ const mouseenter = function mouseenter(
+ this: SVGPathElement,
+ d: GeoFeature,
+ ): void {
// Darken color
let c: string = colorFn(d);
- if (c !== 'none') {
+ if (c) {
c = d3.rgb(c).darker().toString();
}
d3.select(this).style('fill', c);
- // Display information popup
- const result = data.filter(
- region => region.country_id === d.properties.ISO,
- );
- hoverPopup.style('display', 'block').html(
- `<div><strong>${getNameOfRegion(d)}</strong><br>${result.length > 0 ? formatter(result[0].metric) : ''}</div>`,
- );
+ // Display information popup
+ const result = data.filter(r => r.country_id === d?.properties?.ISO);
+ hoverPopup
+ .style('display', 'block')
+ .html(
+ `<div><strong>${getNameOfRegion(d)}</strong><br>${result.length > 0 ? formatter(result[0].metric) : ''}</div>`,
+ );
Review Comment:
**Suggestion:** The tooltip HTML is built with raw region text and injected via `.html(...)` without escaping, which allows script/markup injection if map properties ever contain unsafe strings. Use escaped text (or text nodes) for region names and formatted values before rendering tooltip content. [security]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Malicious GeoJSON labels can execute script on hover.
- β οΈ Country Map tooltips render unescaped region labels and metrics.
- β οΈ Inconsistent escaping compared with other Country Map messages.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. The Country Map chart loads GeoJSON for the selected country via `d3.json(url, (error,
mapData) => { ... })` in
`superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:47-58`, where
`url` comes from the local `countries` mapping (`countries.ts:20-260`) and each feature
carries `NAME_1`/`NAME_2` properties used as region labels.
2. When a user hovers a region, the `mouseenter` handler in `CountryMap.ts:242-261`
filters `data` to find the metric for `d?.properties?.ISO` and then builds the tooltip by
calling `hoverPopup.html(...)` with a template string at `CountryMap.ts:255-259` that
interpolates `getNameOfRegion(d)` and `formatter(result[0].metric)`.
3. `getNameOfRegion` (`CountryMap.ts:220-230`) returns `feature.properties.NAME_2` or
`NAME_1` verbatim with no escaping, and the metric formatter is produced in
`transformProps.ts:50-60` without any HTML sanitization. Although an `escapeHtml` helper
exists and is used for error messages (`CountryMap.ts:33-40`, `CountryMap.ts:40-52`), it
is not applied to these tooltip values.
4. If any GeoJSON feature's `NAME_1`/`NAME_2` (or formatted metric) contains HTML or
script markup (for example, modifying one of the `*.geojson` files under
`superset-frontend/plugins/legacy-plugin-chart-country-map/src/countries/` to set `NAME_1`
to `<img src=x onerror=alert(1)>`), hovering that region will cause `hoverPopup.html(...)`
to inject that markup directly into the DOM, resulting in script execution within the
Superset UI.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2d265fe1c4049f5b88de3591942bbb9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d2d265fe1c4049f5b88de3591942bbb9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts
**Line:** 255:259
**Comment:**
*Security: The tooltip HTML is built with raw region text and injected via `.html(...)` without escaping, which allows script/markup injection if map properties ever contain unsafe strings. Use escaped text (or text nodes) for region names and formatted values before rendering tooltip content.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35859&comment_hash=84ebe7441195db13e56f4c1c79d53dc85e6b5e0b96de41c73bc23b2ae2641f70&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35859&comment_hash=84ebe7441195db13e56f4c1c79d53dc85e6b5e0b96de41c73bc23b2ae2641f70&reaction=dislike'>π</a>
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -112,46 +146,76 @@ function CountryMap(element: HTMLElement, props: CountryMapProps) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
+
+ // Only set grab cursor if not in edit mode
+ if (!isEditMode) {
+ svg.style('cursor', 'grab');
+ }
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);
+ // Add hover popup for tooltip
const hoverPopup = div.append('div').attr('class', 'hover-popup');
- let centered: GeoFeature | null;
-
- const clicked = function clicked(d: GeoFeature) {
- const hasCenter = d && centered !== d;
- let x: number;
- let y: number;
- let k: number;
- 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;
- }
+ // Track mouse position to distinguish clicks from drags
+ let mousedownPos: { x: number; y: number } | null = null;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
- `translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
+ // Cross-filter support
+ const getCrossFilterDataMask = (
+ source: GeoFeature,
+ ): { dataMask: DataMask; isCurrentValueSelected: boolean } | undefined => {
+ if (!entity) return undefined;
+
+ const selected = filterState?.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
+
+ const isSelected = selected.includes(iso);
+ const values = isSelected ? [] : [iso];
Review Comment:
**Suggestion:** Multi-select cross-filtering is not actually implemented: the selection logic always collapses to either one ISO code or none, so Shift+Click cannot add/remove from an existing selection set. This will make users lose prior selections on every click. Update the click/data-mask logic to branch on the pointer event's Shift state and merge/toggle within `filterState.selectedValues` instead of forcing a single-value array. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ Country Map cross-filters collapse to a single region.
- β οΈ Users lose previous region selection on each click.
- β οΈ Dashboard filters less expressive than PR description promises.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open a dashboard containing a Country Map chart (plugin registered in
`superset-frontend/plugins/legacy-plugin-chart-country-map/src/index.ts:30-66`),
configured with an `entity` column and cross-filtering enabled so `emitCrossFilters`,
`filterState`, and `setDataMask` are passed via `transformProps`
(`transformProps.ts:21-32`, `transformProps.ts:62-79`).
2. In view mode, click any region on the Country Map. The SVG path click handler in
`CountryMap.ts:400-405` calls `handleClick(feature)` defined at `CountryMap.ts:352-367`,
which in turn calls `getCrossFilterDataMask(feature)` (`CountryMap.ts:168-193`) and then
invokes `setDataMask(dataMask)`.
3. Inspect `getCrossFilterDataMask` in `CountryMap.ts:168-193`: it reads the current
`filterState?.selectedValues || []` into `selected` (`CountryMap.ts:173`), but then
computes `values` as either `[]` or `[iso]` (`CountryMap.ts:177-178`), discarding any
other previously selected ISO codes. `filterState.selectedValues` in the emitted
`dataMask` is therefore always `null` or a single-element array.
4. With one region already selected, Shift+Click a second region. Because the click
handler in `CountryMap.ts:400-405` does not examine `d3.event` or any `shiftKey` state,
and `handleClick` (`CountryMap.ts:352-367`) always replaces the selection (`newSelection`
is `[]` or `[iso]` at `CountryMap.ts:364-366`), the second click clears the first
selection instead of toggling it, so multi-select cross-filtering as described in the PR
cannot work.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9cd3bcf77f0b44cfb94dd968184be65b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9cd3bcf77f0b44cfb94dd968184be65b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts
**Line:** 177:178
**Comment:**
*Incomplete Implementation: Multi-select cross-filtering is not actually implemented: the selection logic always collapses to either one ISO code or none, so Shift+Click cannot add/remove from an existing selection set. This will make users lose prior selections on every click. Update the click/data-mask logic to branch on the pointer event's Shift state and merge/toggle within `filterState.selectedValues` instead of forcing a single-value array.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35859&comment_hash=7e03a4bf6fb8edc71bc9a9136de2f738d634120a08ac773e5978c6d139abfd6d&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35859&comment_hash=7e03a4bf6fb8edc71bc9a9136de2f738d634120a08ac773e5978c6d139abfd6d&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(country-map): add cross-filters support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35859:
URL: https://github.com/apache/superset/pull/35859#issuecomment-4787082068
We did it! Thanks for the 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org