SBIN2010 commented on code in PR #44562:
URL: https://github.com/apache/superset/pull/44562#discussion_r4098591838
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gantt/EchartsGantt.tsx:
##########
@@ -35,32 +37,43 @@ export default function EchartsGantt(props:
EchartsGanttChartTransformedProps) {
formData,
setControlValue,
onLegendStateChanged,
+ onLegendScroll,
} = props;
const extraControlRef = useRef<HTMLDivElement>(null);
+ const legendStateFrameRef = useRef<number>();
const [extraHeight, setExtraHeight] = useState(0);
useEffect(() => {
const updatedHeight = extraControlRef.current?.offsetHeight ?? 0;
setExtraHeight(updatedHeight);
}, [formData.showExtraControls]);
- const eventHandlers: EventHandlers = {
- legendselectchanged: payload => {
- requestAnimationFrame(() => {
- onLegendStateChanged?.(payload.selected);
- });
- },
- legendselectall: payload => {
- requestAnimationFrame(() => {
- onLegendStateChanged?.(payload.selected);
- });
+ useEffect(
+ () => () => {
+ if (legendStateFrameRef.current !== undefined) {
+ cancelAnimationFrame(legendStateFrameRef.current);
+ }
},
- legendinverseselect: payload => {
- requestAnimationFrame(() => {
- onLegendStateChanged?.(payload.selected);
+ [],
+ );
+
+ const deferLegendStateChange = useCallback(
+ (legendState: LegendState) => {
+ if (legendStateFrameRef.current !== undefined) {
+ cancelAnimationFrame(legendStateFrameRef.current);
+ }
+ legendStateFrameRef.current = requestAnimationFrame(() => {
+ legendStateFrameRef.current = undefined;
+ onLegendStateChanged?.(legendState);
});
},
Review Comment:
The `useLegendEventHandlers` hook itself already delays single-click
processing by 300 ms using `setTimeout` (to allow time for a double-click to
occur). Requesting an animation frame for this 300 ms delay is technically
pointless; in the case of the "All / Invert" buttons, it merely introduces an
unnecessary delay. `requestAnimationFrame` itself is redundant and can be
removed by passing `onLegendStateChanged` directly.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -803,14 +818,20 @@ export function getLegendProps(
legendState?: LegendState,
padding?: LegendPaddingType,
): LegendComponentOption {
- const legend: LegendComponentOption = {
+ // `animation` is read by ECharts but missing from its legend option type
+ const legend: LegendComponentOption & { animation?: boolean } = {
orient: [LegendOrientation.Top, LegendOrientation.Bottom].includes(
orientation,
)
? 'horizontal'
: 'vertical',
show,
type,
+ // A scrolling legend is rebuilt from its first page on every re-render and
+ // then animated back to `scrollDataIndex`, which reads as the legend
+ // sliding away and returning. Turning the animation off makes it render on
+ // the right page to begin with.
+ animation: false,
Review Comment:
This property applies to all legends across all Superset charts. It works
perfectly for `type: 'scroll'`. However, for standard (plain) legends, it
globally disables all transition animations (such as the smooth fading in or
out of legend elements when data changes). It might be worth applying
`animation: false` conditionally—only when `type === 'scroll'`.
--
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]
