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/04/27 02:35:12 UTC

[GitHub] [incubator-echarts] wf123537200 opened a new pull request #12504: feat(tooltip): tooltip formatter trigger once

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


   ## Brief Information
   
   add config as follow to control tooltip.formatter function trigger only once or not
   ```
   tooltip: {
                   triggerOnce: true,        // 如果配置为true，formatter函数只触发一次
                   ...
               },
   ```
   
   This pull request is in the type of:
   
   - [ ] bug fixing
   - [x] new feature
   - [ ] others
   
   
   
   ### What does this PR do?
   
   Control whether the tooltip.formatter function is triggered only once through configuration items, used to reduce the number of high-cost function executions
   
   ### Fixed issues
   
   #12338 
   
   ## Details
   
   ### Before: What was the problem?
   the formatter function trigger many times
   
   ![image](https://user-images.githubusercontent.com/5130528/80328619-675cb680-8872-11ea-8db8-f3dfe42a84c5.png)
   
   
   ### After: How is it fixed in this PR?
   when set triggerOnce: true， each formatter function trigger only once
   
   ![image](https://user-images.githubusercontent.com/5130528/80328554-409e8000-8872-11ea-8736-84215b8ab96e.png)
   
   
   ## Usage
   
   ### Are there any API changes?
   
   - [x] The API has been changed.
   
   ```
   tooltip: {
                   triggerOnce: true,        // 如果配置为true，formatter函数只触发一次
                   ...
               },
   ```
   
   ### Related test cases or examples to use the new APIs
   
   /test/tooltip-formatter-triggerOnce.html
   
   ## Others
   
   ### Merging options
   
   - [ ] Please squash the commits into a single one when merge.
   
   ### Other information
   


----------------------------------------------------------------
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] wf123537200 commented on a change in pull request #12504: feat(tooltip): tooltip formatter trigger once

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



##########
File path: src/component/tooltip/TooltipView.js
##########
@@ -586,7 +610,8 @@ export default echarts.extendComponentView({
                 }
             }, this);
             this._ticket = asyncTicket;
-            html = formatter(params, asyncTicket, callback);
+            html = cacheHtml || formatter(params, asyncTicket, callback);
+            this._setTooltipCache(key, html);

Review comment:
       From a semantic perspective, it is better to wrap




----------------------------------------------------------------
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 #12504: feat(tooltip): tooltip formatter trigger once

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



##########
File path: src/component/tooltip/TooltipView.js
##########
@@ -586,7 +610,8 @@ export default echarts.extendComponentView({
                 }
             }, this);
             this._ticket = asyncTicket;
-            html = formatter(params, asyncTicket, callback);
+            html = cacheHtml || formatter(params, asyncTicket, callback);
+            this._setTooltipCache(key, html);

Review comment:
       If `triggerOnce` is not enabled, `enableCache` will be `false`, so there is no need to call `_setTooltipCache`.
   We should wrap this code line with `if`.




----------------------------------------------------------------
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] wf123537200 commented on pull request #12504: feat(tooltip): tooltip formatter trigger once

Posted by GitBox <gi...@apache.org>.
wf123537200 commented on pull request #12504:
URL: https://github.com/apache/incubator-echarts/pull/12504#issuecomment-619888398


   @plainheart thinks for the suggest,and sorry about spelling mistake, i will Implemented in the dispose function late.
   i think if the user do these in their own code may a little complicated
   if set the cache ```true``` the performance should better then set ```false```


----------------------------------------------------------------
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 #12504: feat(tooltip): tooltip formatter trigger once

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



##########
File path: test/axis-interval-splitLine.html
##########
@@ -0,0 +1,421 @@
+<!DOCTYPE html>

Review comment:
       Maybe `test/axis-interval-splitLine.html` also should be removed.




----------------------------------------------------------------
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] wf123537200 commented on pull request #12504: feat(tooltip): tooltip formatter trigger once

Posted by GitBox <gi...@apache.org>.
wf123537200 commented on pull request #12504:
URL: https://github.com/apache/incubator-echarts/pull/12504#issuecomment-654628079


   @plainheart 3q 4 reviewed, rollback the no need code in 12487.


----------------------------------------------------------------
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 pull request #12504: feat(tooltip): tooltip formatter trigger once

Posted by GitBox <gi...@apache.org>.
plainheart commented on pull request #12504:
URL: https://github.com/apache/incubator-echarts/pull/12504#issuecomment-619916207


   Please add a semicolon `;` at each line end to keep the code style consistent.


----------------------------------------------------------------
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 #12504: feat(tooltip): tooltip formatter trigger once

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


   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] plainheart commented on a change in pull request #12504: feat(tooltip): tooltip formatter trigger once

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



##########
File path: src/component/axis/CartesianAxisView.js
##########
@@ -109,7 +109,14 @@ var CartesianAxisView = AxisView.extend({
         var p2 = [];
 
         var lineStyle = lineStyleModel.getLineStyle();
+        // the showAsAxis will set lineColor to 'transparent' when the viewLabels value is undefind
+        var showAsAxis = splitLineModel.get('showAsAxis');
+        var viewLabels = showAsAxis && axis.getViewLabels();
+
         for (var i = 0; i < ticksCoords.length; i++) {
+            if(viewLabels && viewLabels[i] && !viewLabels[i].formattedLabel) {
+                continue;
+            }

Review comment:
       It seems that the changes in this file are related to #12487, which should not be here.




----------------------------------------------------------------
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 #12504: feat(tooltip): tooltip formatter trigger once

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



##########
File path: src/component/tooltip/TooltipView.js
##########
@@ -586,7 +610,8 @@ export default echarts.extendComponentView({
                 }
             }, this);
             this._ticket = asyncTicket;
-            html = formatter(params, asyncTicket, callback);
+            html = cacheHtml || formatter(params, asyncTicket, callback);
+            this._setTooltipCache(key, html);

Review comment:
       Maybe no need to wrap, I saw that it won't invoke if `key` is not specified in function `_setTooltipCache`.




----------------------------------------------------------------
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 pull request #12504: feat(tooltip): tooltip formatter trigger once

Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #12504:
URL: https://github.com/apache/incubator-echarts/pull/12504#issuecomment-668460926


   @wf123537200 Sorry for the late review. Some of my thoughts about this pull request:
   
   1. `triggerOnce` should be a more specific name like `formatterCacheResult`. Or it's easy to be mistaken that this tooltip will only show once.
   2. params can be a single object when the trigger is `'item'` in https://github.com/apache/incubator-echarts/pull/12504/files#diff-d5fbb7185a3802482df65d3d3a345c70R598
   3. The key generation logic may have issues in some cases, for example, the key won't change if only the number of series changes. 
   4. Key generation seems to depend on the business logic. So should we add a new option called `formatterCacheKey`? Which is a callback that receives same parameter with formatter and returns a key string.
   
   


----------------------------------------------------------------
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] wf123537200 commented on pull request #12504: feat(tooltip): tooltip formatter trigger once

Posted by GitBox <gi...@apache.org>.
wf123537200 commented on pull request #12504:
URL: https://github.com/apache/incubator-echarts/pull/12504#issuecomment-619921175


   > Please add a semicolon `;` at each line end to keep the code style consistent.
   
   Thank you for reminding, already use the ```npm run lint``` check the code style consistent 
   


----------------------------------------------------------------
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 edited a comment on pull request #12504: feat(tooltip): tooltip formatter trigger once

Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #12504:
URL: https://github.com/apache/incubator-echarts/pull/12504#issuecomment-668460926


   @wf123537200 Sorry for the late review. Some of my thoughts about this pull request:
   
   1. `triggerOnce` should be a more specific name like `formatterCache`. Or it's easy to be mistaken that this tooltip will only show once.
   2. params can be a single object when the trigger is `'item'` in https://github.com/apache/incubator-echarts/pull/12504/files#diff-d5fbb7185a3802482df65d3d3a345c70R598
   3. The key generation logic may have issues in some cases, for example, the key won't change if only the number of series changes. 
   4. Key generation seems to depend on the business logic. So should we add a new option called `formatterCacheKey`? Which is a callback that receives same parameter with formatter and returns a key string.
   
   


----------------------------------------------------------------
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 #12504: feat(tooltip): tooltip formatter trigger once

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



##########
File path: src/component/tooltip/TooltipView.js
##########
@@ -586,7 +610,8 @@ export default echarts.extendComponentView({
                 }
             }, this);
             this._ticket = asyncTicket;
-            html = formatter(params, asyncTicket, callback);
+            html = cacheHtml || formatter(params, asyncTicket, callback);
+            this._setTooltipCache(key, html);

Review comment:
       If `triggerOnce` is not enabled, `enableCache` will be false, so there is no need to call `_setTooltipCache`.
   We should wrapped this code line with `if`.

##########
File path: src/component/tooltip/TooltipView.js
##########
@@ -577,6 +591,16 @@ export default echarts.extendComponentView({
             html = formatUtil.formatTpl(formatter, params, true);
         }
         else if (typeof formatter === 'function') {
+            // formatter cache
+            var cacheHtml = null;
+            var key = null;

Review comment:
       Some varaibles have no need to be assigned an initial value `null`.
   ```suggestion
               var cacheHtml;
               var key;
   ```

##########
File path: src/component/tooltip/TooltipView.js
##########
@@ -577,6 +591,16 @@ export default echarts.extendComponentView({
             html = formatUtil.formatTpl(formatter, params, true);
         }
         else if (typeof formatter === 'function') {
+            // formatter cache
+            var cacheHtml = null;
+            var key = null;
+            var enableCache = tooltipModel.get('triggerOnce');
+            if (enableCache && params && params[0]) {
+                var p = params[0];
+                key = [p.componentIndex, p.seriesId, p.seriesName, p.dataIndex].join('_');
+                cacheHtml = this._getTooltipCache(key);
+            }
+
             var callback = bind(function (cbTicket, html) {
                 if (cbTicket === this._ticket) {
                     tooltipContent.setContent(html, markers, tooltipModel);

Review comment:
       The case of `callback` in `formatter` also needs to be handled. Add cache here ?




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