Re: [PR] feat(line): add `triggerEvent` option for more control over mouse event [echarts]

["plainheart (via GitHub)" <gi...@apache.org>]

plainheart commented on code in PR #21001:
URL: https://github.com/apache/echarts/pull/21001#discussion_r2147965904


##########
src/chart/line/LineSeries.ts:
##########
@@ -213,7 +215,11 @@ class LineSeriesModel extends SeriesModel<LineSeriesOption> {
             divideShape: 'clone'
         },
 
-        triggerLineEvent: false
+        // Whether to trigger event when hovering on either line or area.
+        triggerLineEvent: false,
+
+        // Whether to trigger event when hovering only on line.
+        triggerLineOnlyEvent: false

Review Comment:
   To be honest, `triggerLineOnlyEvent` is more confusing. I don't know what it means until I see this comment. If we want to refine the `triggerEvent` behavior to split it into the line event and area event, I would suggest introducing a new option `triggerEvent` - the `triggerEvent` ‌option is used repeatedly. For instance, `title.triggerEvent`, `xAxis.triggerEvent`, etc.
   
   So I think we can merge both options into a single option `triggerEvent` with the boolean type and constant string `'line'/'area'` allowed.
   
   For backward compatibility, the new option `triggerEvent` should work with both line and area unless it is explicitly specified to a single target.
   
   The changes look like this,
   
   **_src/chart/line/LineSeries.ts_**
   
   ```ts
   export interface LineSeriesOption extends SeriesOption<LineStateOption<CallbackDataParams>, LineStateOptionMixin & {
       // ...
   
       /**
        * @deprecated Use `triggerEvent: 'line'` for only line event or `triggerEvent: true` for both line and area event. 
        */
       triggerLineEvent?: boolean
   
      /**
       * Whether to trigger event when hovering on the line or the area
       * @since v6.0.0
       */
       triggerEvent?: boolean | 'line' | 'area'
   }
   
   class LineSeriesModel extends SeriesModel<LineSeriesOption> {
       // ...
   
       /**
        * @deprecated
        */
       triggerLineEvent: false,
   
       triggerEvent: false
   };
   ```
   
   **_src/chart/line/LineView.ts_**
   
   Note that `triggerLineEvent` has higher priority than `triggerEvent` for compatibility.
   
   ```ts
   const triggerEvent = seriesModel.get('triggerEvent');
   const triggerLineEvent = seriesModel.get('triggerLineEvent');
   
   const shouldTriggerLineEvent = triggerLineEvent === true || triggerEvent === true || triggerEvent === 'line';
   const shouldTriggerAreaEvent = triggerLineEvent === true || triggerEvent === true || triggerEvent === 'area';
   
   shouldTriggerLineEvent && this.packEventData(seriesModel, polyline);
   shouldTriggerAreaEvent && polygon && this.packEventData(seriesModel, polygon);
   ```
   
   But `triggerEvent` does not control whether to trigger the symbol event, which is triggered by default. This may be a confusing point, so it's better to add some explanation. I would like to hear what you think about 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: commits-unsubscribe@echarts.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org