You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2020/05/04 09:43:38 UTC

[GitHub] [incubator-echarts] plainheart opened a new pull request #12561: fix(pie): pie series render incorrectly after editing its data in DataView. close #12560.

plainheart opened a new pull request #12561:
URL: https://github.com/apache/incubator-echarts/pull/12561


   
   ## Brief Information
   
   This pull request is in the type of:
   
   - [x] bug fixing
   - [ ] new feature
   - [ ] others
   
   
   
   ### What does this PR do?
   
   - 1. Fixes the issue #12560.
   - 2. Fixes the bug that pie series will be rendered incorrectly when user forget to delete `\n` or retained some spaces in `DataView`.
   
   ### Fixed issues
   
   - #12560
   
   ### Code to Reproduce: 
   
   - case 1. Click the `DataView` tool at the top-right cornor, and delete the first line `USA`.
   - case 2. Delete the last line (but don't remove `\n` or  just retain some spaces).
   
   ## Details
   
   ### Before: 
   
   ![image](https://user-images.githubusercontent.com/26999792/80953000-40335580-8e2d-11ea-921c-064d9e30a42b.png)
   
   ![image](https://user-images.githubusercontent.com/26999792/80953669-9359d800-8e2e-11ea-9d6e-5b441c7859b9.png)
   
   ### After: 
   
   ![image](https://user-images.githubusercontent.com/26999792/80953033-50e3cb80-8e2d-11ea-8257-b33b936a9ef0.png)
   
   ![image](https://user-images.githubusercontent.com/26999792/80953721-a8cf0200-8e2e-11ea-862c-4cb499ab4865.png)
   
   ## Usage
   
   ### Related test cases or examples to use the new APIs
   
   Refer to `test/pie-dataView.html`
   


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

[GitHub] [incubator-echarts] Ovilia commented on a change in pull request #12561: fix(pie): pie series render incorrectly after editing its data in DataView. close #12560.

Posted by GitBox <gi...@apache.org>.
Ovilia commented on a change in pull request #12561:
URL: https://github.com/apache/incubator-echarts/pull/12561#discussion_r420526638



##########
File path: src/component/toolbox/feature/DataView.js
##########
@@ -426,13 +432,17 @@ function tryMergeDataOption(newData, originalData) {
     return zrUtil.map(newData, function (newVal, idx) {
         var original = originalData && originalData[idx];
         if (zrUtil.isObject(original) && !zrUtil.isArray(original)) {
-            if (zrUtil.isObject(newVal) && !zrUtil.isArray(newVal)) {
-                newVal = newVal.value;
+            var _newVal;
+            if (zrUtil.isArray(newVal) || zrUtil.isObject(newVal)) {

Review comment:
       Thanks for your contribution.
   
   Could you explain the change here?
   
   And please do not use `_` in variable names.




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

[GitHub] [incubator-echarts] pissang commented on a change in pull request #12561: fix(pie): pie series render incorrectly after editing its data in DataView. close #12560.

Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12561:
URL: https://github.com/apache/incubator-echarts/pull/12561#discussion_r464363470



##########
File path: src/component/toolbox/feature/DataView.js
##########
@@ -426,13 +432,18 @@ function tryMergeDataOption(newData, originalData) {
     return zrUtil.map(newData, function (newVal, idx) {
         var original = originalData && originalData[idx];
         if (zrUtil.isObject(original) && !zrUtil.isArray(original)) {
-            if (zrUtil.isObject(newVal) && !zrUtil.isArray(newVal)) {
-                newVal = newVal.value;
+            var newValIsObject = zrUtil.isObject(newVal) && !zrUtil.isArray(newVal);
+            if (!newValIsObject) {
+                newVal = {
+                    value: newVal
+                };
+            }
+            // original data has name but new data has no name
+            if (original.name != null && newVal.name == null) {
+                delete original.name;
             }
             // Original data has option

Review comment:
       Should not modify the original data. It's better to delete the name in newVal after `defaults`




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

[GitHub] [incubator-echarts] plainheart commented on a change in pull request #12561: fix(pie): pie series render incorrectly after editing its data in DataView. close #12560.

Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #12561:
URL: https://github.com/apache/incubator-echarts/pull/12561#discussion_r420661647



##########
File path: src/component/toolbox/feature/DataView.js
##########
@@ -426,13 +432,17 @@ function tryMergeDataOption(newData, originalData) {
     return zrUtil.map(newData, function (newVal, idx) {
         var original = originalData && originalData[idx];
         if (zrUtil.isObject(original) && !zrUtil.isArray(original)) {
-            if (zrUtil.isObject(newVal) && !zrUtil.isArray(newVal)) {
-                newVal = newVal.value;
+            var _newVal;
+            if (zrUtil.isArray(newVal) || zrUtil.isObject(newVal)) {

Review comment:
       Thanks for your review and sorry for the wrong variable name defined before.
   
   The reason for making these changes is to ensure more compatibility at first.
   
   For the case: **Delete any data item in any line in GridView**
   
   In original logic context, it only handles the change of `value` but does not handle the change of `name`. Therefore, after any data item line is deleted, final merged result will ignore `name`.
   
   An example:
   **original data items:**
   ```js
   [
       { name:  'USA', value: 15276 },
       { name:  'UK', value: 4855 }
   ]
   ```
   
   **the first in original data items** 
   `{ name: 'USA', value: 15276 }`
   
   **the first data item after the old first data item was deleted(the second became the first)**
   `{  name:  "UK", value: 4855  }`
   
   After `tryMergeDataOption`, data items became
   `[ { name: 'USA', value: 4855 } ]`
   
   The result above is not obviously expected.
   Both `name` and `value` of each data item should be handled and merged well.
   
   By now, I think there should be some changes here.
   ```js
   function tryMergeDataOption(newData, originalData) {
       return zrUtil.map(newData, function (newVal, idx) {
           var original = originalData && originalData[idx];
           if (zrUtil.isObject(original) && !zrUtil.isArray(original)) {
               var newValIsObject = zrUtil.isObject(newVal) && !zrUtil.isArray(newVal);
               if (!newValIsObject) {
                   newVal = {
                       value: newVal
                   };
               }
               if (original.name != null && newVal.name == null) {
                   // original data has name but new data has no name
                   delete original.name;
               }
               // Original data has option
               return zrUtil.defaults(newVal, original);
           }
           else {
               return newVal;
           }
       });
   }
   ```
   
   




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

[GitHub] [incubator-echarts] pissang merged pull request #12561: fix(pie): pie series render incorrectly after editing its data in DataView. close #12560.

Posted by GitBox <gi...@apache.org>.
pissang merged pull request #12561:
URL: https://github.com/apache/incubator-echarts/pull/12561


   


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

[GitHub] [incubator-echarts] echarts-bot[bot] commented on pull request #12561: fix(pie): pie series render incorrectly after editing its data in DataView. close #12560.

Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #12561:
URL: https://github.com/apache/incubator-echarts/pull/12561#issuecomment-623364495


   Thanks for your contribution!
   The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/incubator-echarts/wiki/How-to-make-a-pull-request).


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

[GitHub] [incubator-echarts] echarts-bot[bot] commented on pull request #12561: fix(pie): pie series render incorrectly after editing its data in DataView. close #12560.

Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #12561:
URL: https://github.com/apache/incubator-echarts/pull/12561#issuecomment-668411358


   Congratulations! Your PR has been merged. Thanks for your contribution! 👍


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

[GitHub] [incubator-echarts] plainheart commented on a change in pull request #12561: fix(pie): pie series render incorrectly after editing its data in DataView. close #12560.

Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #12561:
URL: https://github.com/apache/incubator-echarts/pull/12561#discussion_r420661647



##########
File path: src/component/toolbox/feature/DataView.js
##########
@@ -426,13 +432,17 @@ function tryMergeDataOption(newData, originalData) {
     return zrUtil.map(newData, function (newVal, idx) {
         var original = originalData && originalData[idx];
         if (zrUtil.isObject(original) && !zrUtil.isArray(original)) {
-            if (zrUtil.isObject(newVal) && !zrUtil.isArray(newVal)) {
-                newVal = newVal.value;
+            var _newVal;
+            if (zrUtil.isArray(newVal) || zrUtil.isObject(newVal)) {

Review comment:
       Thanks for your review and sorry for the wrong variable name defined before.
   
   The reason for making these changes is to ensure more compatibility at first.
   
   For the case: **Delete any data item in any line in GridView**
   
   In original logic context, it only handles the change of `value` but does not handle the change of `name`. Therefore, after any data item line is deleted, final merged result will ignore `name`.
   
   An example:
   **original data items:**
   ```js
   [
       { name:  'USA', value: 15276 },
       { name:  'UK', value: 4855 }
   ]
   ```
   
   **the first in original data items** 
   `{ name: 'USA', value: 15276 }`
   
   **the first data item after the old first data item was deleted(the second became the first)**
   `{  name:  "UK", value: 4855  }`
   
   After `tryMergeDataOption`, data items became
   `[ { name: 'USA', value: 4855 } ]`
   
   The result above is not obviously expected.
   Both `name` and `value` of each data item should be handled and merged well.
   
   By now, I think there should be some changes here.
   ```js
   function tryMergeDataOption(newData, originalData) {
       return zrUtil.map(newData, function (newVal, idx) {
           var original = originalData && originalData[idx];
           if (zrUtil.isObject(original) && !zrUtil.isArray(original)) {
               var newValIsObject = zrUtil.isObject(newVal) && !zrUtil.isArray(newVal);
               if (!newValIsObject) {
                   newVal = {
                       value: newVal
                   };
               }
               if (original.name != null && newVal.name == null) {
                   // original data has name but new data has no name
                   delete original.name;
               }
               // Original data has option
               return zrUtil.defaults(newVal, original);
           }
           else {
               return newVal;
           }
       });
   }
   ```
   
   Finally, I found another bug that it will occur after editing the series name in DataView.
   
   ![image](https://user-images.githubusercontent.com/26999792/81162277-7ef91500-8fbf-11ea-8784-f356ccd8e74b.png)
   
   
   




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

[GitHub] [incubator-echarts] plainheart commented on a change in pull request #12561: fix(pie): pie series render incorrectly after editing its data in DataView. close #12560.

Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #12561:
URL: https://github.com/apache/incubator-echarts/pull/12561#discussion_r464831943



##########
File path: src/component/toolbox/feature/DataView.js
##########
@@ -426,13 +432,18 @@ function tryMergeDataOption(newData, originalData) {
     return zrUtil.map(newData, function (newVal, idx) {
         var original = originalData && originalData[idx];
         if (zrUtil.isObject(original) && !zrUtil.isArray(original)) {
-            if (zrUtil.isObject(newVal) && !zrUtil.isArray(newVal)) {
-                newVal = newVal.value;
+            var newValIsObject = zrUtil.isObject(newVal) && !zrUtil.isArray(newVal);
+            if (!newValIsObject) {
+                newVal = {
+                    value: newVal
+                };
+            }
+            // original data has name but new data has no name
+            if (original.name != null && newVal.name == null) {
+                delete original.name;
             }
             // Original data has option

Review comment:
       Yes, I missed it before. I've tweaked this logic, please help review, thank you.




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