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/10/10 06:57:38 UTC
[GitHub] [incubator-echarts] plainheart commented on a change in pull request #13398: fix(tooltip): fix some issues in new tooltip.
plainheart commented on a change in pull request #13398:
URL: https://github.com/apache/incubator-echarts/pull/13398#discussion_r502756112
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -386,8 +392,9 @@ class TooltipHTMLContent {
}
hide() {
- this.el.style.display = 'none';
+ this.el.style.opacity = '0';
this._show = false;
+ this._longHideTimeout = setTimeout(() => this._longHide = true, 500) as any;
}
Review comment:
> I think we still need to set `display: 'none'` when `this._longHide = true`. I'm not sure if it will cause other issues like cover the graph and cause interaction issues. if only set the opacity to `0`.
Perhaps, you are worried that it will cause interaction issues if only set `opacity` to `0` for `opacity: 0` will still respond to DOM events. I suppose this issue could be solved through setting `visibility` to `hidden`. It won't respond to DOM events. So it should not bring interaction issues anymore. See this commit f038591383293467900c791865f4cb284c315acf
> Also, should the time `500ms` plus user set `hideDelay`?
This logic is now in the `hide` function, `hide` is called by `hideLater`, and `hideLater` has applied the `hideDelay`.
So I think no need to plus the specified `hideDelay` here again.
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -386,8 +392,9 @@ class TooltipHTMLContent {
}
hide() {
- this.el.style.display = 'none';
+ this.el.style.opacity = '0';
this._show = false;
+ this._longHideTimeout = setTimeout(() => this._longHide = true, 500) as any;
}
Review comment:
> I think we still need to set `display: 'none'` when `this._longHide = true`. I'm not sure if it will cause other issues like cover the graph and cause interaction issues. if only set the opacity to `0`.
Perhaps, you are worried that it will cause interaction issues if only set `opacity` to `0` for `opacity: 0` will still respond to DOM events. I suppose this issue could be solved through setting `visibility` to `hidden`. It won't respond to DOM events. So it should not bring interaction issues anymore and in the meanwhile, it can keep the fade transition when the tooltip is hidden. See this commit f038591383293467900c791865f4cb284c315acf
> Also, should the time `500ms` plus user set `hideDelay`?
This logic is now in the `hide` function, `hide` is called by `hideLater`, and `hideLater` has applied the `hideDelay`.
So I think no need to plus the specified `hideDelay` here again.
----------------------------------------------------------------
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