Copilot commented on code in PR #37022:
URL: https://github.com/apache/superset/pull/37022#discussion_r2683465272
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/tooltip.ts:
##########
@@ -27,12 +27,21 @@ import {
import { TOOLTIP_OVERFLOW_MARGIN, TOOLTIP_POINTER_MARGIN } from '../constants';
import { Refs } from '../types';
+const TOOLTIP_Z_INDEX = 2147483647;
Review Comment:
The z-index value 2147483647 (max 32-bit signed integer) is extremely high
and may cause issues. Most browsers support z-index values up to 2147483647,
but such an extreme value can lead to:
1. Stacking context conflicts with browser chrome or extensions
2. Difficulty debugging layering issues
3. No room for future UI elements that need to be above this
Looking at the codebase, z-index values typically range from 1-3000, with
modals and popovers using values around 1000-3000. Consider using a more
reasonable value that still ensures the tooltip appears above the sticky header
(which uses z-index: 100) and other dashboard elements. A value in the range of
3000-10000 would be more appropriate and maintainable.
```suggestion
const TOOLTIP_Z_INDEX = 10000;
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/tooltip.ts:
##########
@@ -27,12 +27,21 @@ import {
import { TOOLTIP_OVERFLOW_MARGIN, TOOLTIP_POINTER_MARGIN } from '../constants';
import { Refs } from '../types';
+const TOOLTIP_Z_INDEX = 2147483647;
+
export function getDefaultTooltip(refs: Refs) {
return {
appendToBody: true,
borderColor: 'transparent',
// CSS hack applied on this class to resolve
https://github.com/apache/superset/issues/30058
className: 'echarts-tooltip',
+ // allow scrolling inside tooltip without re-triggering the chart
+ enterable: true,
+ // keep within viewport and above header; enable internal scroll
+ confine: true,
+ extraCssText: `max-height:80vh; overflow:auto;
-webkit-overflow-scrolling:touch; pointer-events:auto;
z-index:${TOOLTIP_Z_INDEX};`,
+ // optional: reduce flicker when moving in/out of tooltip
+ hideDelay: 50,
Review Comment:
The new tooltip configuration options (enterable, confine, hideDelay, and
extraCssText) significantly change the tooltip behavior but lack test coverage.
Since other utility functions in the same directory have comprehensive test
files, consider adding tests to verify:
1. The tooltip configuration includes the new properties
2. The z-index is correctly interpolated into extraCssText
3. The max-height constraint is properly applied
This would help prevent regressions when the tooltip behavior is modified in
the future.
--
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]
