100pah commented on a change in pull request #10221: Fix #10210 wrong color of 
piecewise visualMap with scatter charts
URL: 
https://github.com/apache/incubator-echarts/pull/10221#discussion_r386429208
 
 

 ##########
 File path: src/data/helper/sourceHelper.js
 ##########
 @@ -664,14 +664,12 @@ function doGuessOrdinal(
     }
 
     function detectValue(val) {
-        var beStr = isString(val);
-        // Consider usage convenience, '1', '2' will be treated as "number".
-        // `isFinit('')` get `true`.
-        if (val != null && isFinite(val) && val !== '') {
-            return beStr ? BE_ORDINAL.Might : BE_ORDINAL.Not;
-        }
-        else if (beStr && val !== '-') {
-            return BE_ORDINAL.Must;
+        // For string, assign its type to 'ordinal'
+        if (isString(val)) {
+            return true;
+        }
+        else if (val != null && isNaN(val)) {
+            return false;
 
 Review comment:
   I am not sure how to fix it neatly and simple.
   
   These factors determine that whether a dimension of a series data is 
category/ordinal:
   + User specified explicitly by `series.dimension = ['xxx', 'yyy', {name: 
'zzz', type: 'ordinal'}]` (see 
<https://echarts.apache.org/en/option.html#series-line.dimensions>)
   + The dimension is mapped to a category axis (most of the related logic is 
in `createListFromArray.js`)
   + Guess ordinal by its value (in `sourceHelper.js`)
   
   The most of the code in `visualMap` is written in early time and some the 
design is not good enough now.
   For example, if setting `visualMap.dimension` to a dimension that mapped to 
a "category axis", and intending to use `visualMap.category`, that does not 
work. Because the data of that dimension has been parse and stored as "ordinal 
index" in `List.js#storage` but visual map use the "ordinal index" to compared 
with the `visualMap.category`, which will not get correct result. However, this 
issue might not need to cover this case.
   
   The possible solution of this issue could be A or B (or others?):
   
   (A) in `src/visual/visualSolution` 
   
https://github.com/apache/incubator-echarts/blob/4.7.0-rc.1/src/visual/visualSolution.js#L211
   
https://github.com/apache/incubator-echarts/blob/4.7.0-rc.1/src/visual/visualSolution.js#L158
   we make some trick that: if the `visualMap.category` is specified or the 
dimension type has been `ordinal`, we retrieve the "raw value" to "applyVisual".
   This fix is not neat but probably simple.
   
   (B) Since if a series data dimension will be detected as type "ordinal" if 
it is mapped to a category axis, we can also take visualMap into account. That 
is, if a series data dimension is referred by a `viusalMap` and the 
`viusalMap.categories` is specified, the dimension should be typed as 
"ordinal". 
   But this fix might change lots of code in "createListFromArray.js", 
"createListSimply.js" (or in "guessOrdinal") and other places.
   
   @pissang What's your opinion?
   

----------------------------------------------------------------
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:
[email protected]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to