[GitHub] [incubator-echarts] pissang commented on a change in pull request #12484: feat: bar race

[GitBox <gi...@apache.org>]

pissang commented on a change in pull request #12484:
URL: https://github.com/apache/incubator-echarts/pull/12484#discussion_r442009191



##########
File path: src/chart/helper/labelHelper.ts
##########
@@ -20,22 +20,27 @@
 
 import {retrieveRawValue} from '../../data/helper/dataProvider';
 import List from '../../data/List';
+import {ParsedValue} from '../../util/types';
 
 /**
  * @return label string. Not null/undefined
  */
-export function getDefaultLabel(data: List, dataIndex: number): string {
+export function getDefaultLabel(
+    data: List,
+    dataIndex: number,
+    interpolatedValues?: ParsedValue | ParsedValue[]
+): string {
     const labelDims = data.mapDimensionsAll('defaultedLabel');
     const len = labelDims.length;
 
     // Simple optimization (in lots of cases, label dims length is 1)
     if (len === 1) {
-        return retrieveRawValue(data, dataIndex, labelDims[0]);
+        return retrieveRawValue(data, dataIndex, labelDims[0], interpolatedValues);
     }

Review comment:
       Perhaps it's more reasonable to be:
    
   ```js
   return interpolatedValues == null ? retrieveRawValue(data, dataIndex, labelDims[0]) : interpolatedValues
   ```

##########
File path: src/scale/Ordinal.ts
##########
@@ -99,6 +103,23 @@ class OrdinalScale extends Scale {
         return;
     }
 
+    setCategorySortInfo(info: OrdinalSortInfo[]): void {
+        this._categorySortInfo = info;
+    }
+

Review comment:
       I can't find the general logic of sorting the categories when `realtimeSort` is not set. Did I miss it?

##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -447,6 +462,36 @@ class Grid implements CoordinateSystemMaster {
                 );
             });
         }
+
+        // function sortCategory(
+        //     data: List,

Review comment:
       Remove the commented code if they are not used anymore

##########
File path: src/model/mixin/dataFormat.ts
##########
@@ -96,13 +99,14 @@ class DataFormatMixin {
         status?: DisplayState,
         dataType?: string,
         dimIndex?: number,
-        labelProp?: string
+        labelProp?: string,
+        interpolateValues?: ParsedValue | ParsedValue[]
     ): string {
         status = status || 'normal';
         const data = this.getData(dataType);
         const itemModel = data.getItemModel(dataIndex);
 
-        const params = this.getDataParams(dataIndex, dataType);
+        const params = this.getDataParams(dataIndex, dataType, null, interpolateValues);
         if (dimIndex != null && (params.value instanceof Array)) {

Review comment:
       I think it's better to modify the `params.value` after getDataParams instead of adding an extra parameter.
   
   Other properties may also have a chance to be modified in the future. 

##########
File path: src/chart/bar/BarView.ts
##########
@@ -316,7 +423,84 @@ class BarView extends ChartView {
         }
     }
 
-    remove(ecModel?: GlobalModel): void {
+    _dataSort(
+        data: List<BarSeriesModel, DefaultDataVisual>,
+        map: ((idx: number) => number)
+    ): OrdinalSortInfo[] {
+        type SortValueInfo = {
+            mappedValue: number,
+            ordinalNumber: OrdinalNumber,
+            beforeSortIndex: number
+        };
+        const info: SortValueInfo[] = [];
+        data.each(idx => {
+            info.push({
+                mappedValue: map(idx),
+                ordinalNumber: idx,
+                beforeSortIndex: null
+            });
+        });
+
+        info.sort((a, b) => {
+            return b.mappedValue - a.mappedValue;
+        });
+
+        // Update beforeSortIndex
+        for (let i = 0; i < info.length; ++i) {
+            info[info[i].ordinalNumber].beforeSortIndex = i;
+        }
+        return info.map(item => {
+            return {

Review comment:
       Use `zrUtil.map` instead of ES5 map

##########
File path: src/coord/cartesian/AxisModel.ts
##########
@@ -35,6 +36,10 @@ interface CartesianAxisOption extends AxisBaseOption {
     position?: CartesianAxisPosition;
     // Offset is for multiple axis on the same position.
     offset?: number;
+    sort?: boolean;
+    realtimeSort?: boolean;
+    sortSeriesIndex?: number;

Review comment:
       Still not very show if it's better to put `sort` `realtimeSort` configuration on the series. Perhaps we can let developers do the decision.




----------------------------------------------------------------
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.

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