Copilot commented on code in PR #37105:
URL: https://github.com/apache/superset/pull/37105#discussion_r2713810204
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -134,6 +134,16 @@ function CountryMap(element, props) {
return '';
};
+ const svgHeight = svg.node().getBoundingClientRect().height;
+ const updatePopupPosition = () => {
+ const [x, y] = d3.mouse(svg.node());
+ hoverPopup
+ .style('display', 'block')
+ .style('top', `${y + 30}px`)
+ .style('left', `${x}px`)
+ .classed('popup-at-bottom', y > (svgHeight * 2) / 3);
+ };
Review Comment:
The new tooltip positioning behavior (especially the popup-at-bottom class
assignment) lacks test coverage. While the existing tests verify basic tooltip
show/hide functionality, they don't verify that the tooltip correctly positions
itself when hovering near the bottom of the map, or that the popup-at-bottom
class is properly applied when y > (svgHeight * 2) / 3. Consider adding test
cases to verify this new positioning logic works correctly.
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -134,6 +134,16 @@ function CountryMap(element, props) {
return '';
};
+ const svgHeight = svg.node().getBoundingClientRect().height;
+ const updatePopupPosition = () => {
+ const [x, y] = d3.mouse(svg.node());
Review Comment:
The svgHeight variable is calculated once at component initialization on
line 137 and won't update if the SVG is resized after initial render. If the
map component can be dynamically resized (e.g., window resize, container
resize), the threshold calculation on line 144 will use stale height values,
causing incorrect tooltip positioning. Consider moving the height calculation
inside updatePopupPosition() or adding a resize listener to update svgHeight.
```suggestion
const updatePopupPosition = () => {
const [x, y] = d3.mouse(svg.node());
const svgHeight = svg.node().getBoundingClientRect().height;
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -154,13 +164,15 @@ function CountryMap(element, props) {
.html(
`<div><strong>${getNameOfRegion(d)}</strong><br>${result.length > 0 ?
format(result[0].metric) : ''}</div>`,
);
+ updatePopupPosition();
};
const mousemove = function mousemove() {
const position = d3.mouse(svg.node());
hoverPopup
.style('top', `${position[1] + 30}px`)
.style('left', `${position[0]}px`);
+ updatePopupPosition();
Review Comment:
The tooltip position is set twice in the mousemove function - once on lines
172-174 and then again via updatePopupPosition() on line 175. This is redundant
code that duplicates positioning logic. Since updatePopupPosition() handles all
positioning including the conditional class assignment, the manual position
setting on lines 172-174 should be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
