Re: [PR] fix(Echarts): Echarts Legend Scroll fix [superset]

[via GitHub]

korbit-ai[bot] commented on code in PR #33779:
URL: https://github.com/apache/superset/pull/33779#discussion_r2150141048


##########
superset-frontend/packages/superset-ui-core/src/chart/models/ChartProps.ts:
##########
@@ -105,6 +107,8 @@ export interface ChartPropsConfig {
   inputRef?: RefObject<any>;
   /** Theme object */
   theme: SupersetTheme;
+  /* legend index */
+  legendIndex?: number;

Review Comment:
   ### Unclear Property Description <sub>![category 
Readability](https://img.shields.io/badge/Readability-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The JSDoc comment for legendIndex uses incorrect comment style and lacks 
meaningful description of what the index represents.
   
   
   ###### Why this matters
   Vague property descriptions force developers to look elsewhere in the 
codebase to understand the purpose of the field.
   
   ###### Suggested change ∙ *Feature Preview*
   ```typescript
   /** Index tracking the current scroll position of the legend */
   legendIndex?: number;
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/41bcccc3-5956-432b-a8e1-9e8a3fe9c0bf/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/41bcccc3-5956-432b-a8e1-9e8a3fe9c0bf?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/41bcccc3-5956-432b-a8e1-9e8a3fe9c0bf?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/41bcccc3-5956-432b-a8e1-9e8a3fe9c0bf?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/41bcccc3-5956-432b-a8e1-9e8a3fe9c0bf)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:92bd6aac-4de7-4d08-b9ba-b87734f3f3f8 -->
   
   
   [](92bd6aac-4de7-4d08-b9ba-b87734f3f3f8)



##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -159,6 +160,9 @@ export default function EchartsTimeseries({
     mouseover: params => {
       onFocusedSeries(params.seriesName);
     },
+    legendscroll: payload => {
+      onLegendScroll?.(payload.scrollDataIndex);
+    },

Review Comment:
   ### Incorrect ECharts Legend Scroll Event Property <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The code assumes payload.scrollDataIndex exists and is the correct property 
for legend scroll data, but ECharts legendscroll event actually provides scroll 
data in payload.start.
   
   
   ###### Why this matters
   Using the wrong property will result in undefined values being passed to the 
handler, causing the legend scroll functionality to fail.
   
   ###### Suggested change ∙ *Feature Preview*
   Update the legendscroll handler to use the correct property:
   ```typescript
   legendscroll: payload => {
         onLegendScroll?.(payload.start);
       },
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd0e6cc7-313b-48b2-8cc2-519aed23fcce/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd0e6cc7-313b-48b2-8cc2-519aed23fcce?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd0e6cc7-313b-48b2-8cc2-519aed23fcce?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd0e6cc7-313b-48b2-8cc2-519aed23fcce?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd0e6cc7-313b-48b2-8cc2-519aed23fcce)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:31282ef6-4638-4459-8065-90bf6d02a1c3 -->
   
   
   [](31282ef6-4638-4459-8065-90bf6d02a1c3)



-- 
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-unsubscr...@superset.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org