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 2021/03/30 04:10:37 UTC
[GitHub] [echarts] pissang commented on a change in pull request #14542: fix(label): fix labels are not on the top bug.
pissang commented on a change in pull request #14542:
URL: https://github.com/apache/echarts/pull/14542#discussion_r603765920
##########
File path: src/core/echarts.ts
##########
@@ -2157,34 +2157,44 @@ class ECharts extends Eventful<ECEventDefinition> {
if (model.preventAutoZ) {
return;
}
- const z = model.get('z');
- const zlevel = model.get('zlevel');
// Set z and zlevel
- view.group.traverse(function (el: Displayable) {
- if (!el.isGroup) {
- z != null && (el.z = z);
- zlevel != null && (el.zlevel = zlevel);
-
- // TODO if textContent is on group.
- const label = el.getTextContent();
- const labelLine = el.getTextGuideLine();
- if (label) {
- label.z = el.z;
- label.zlevel = el.zlevel;
- // lift z2 of text content
- // TODO if el.emphasis.z2 is spcefied, what about textContent.
- label.z2 = el.z2 + 2;
- }
- if (labelLine) {
- const showAbove = el.textGuideLineConfig && el.textGuideLineConfig.showAbove;
- labelLine.z = el.z;
- labelLine.zlevel = el.zlevel;
- labelLine.z2 = el.z2 + (showAbove ? 1 : -1);
- }
- }
- });
+ _updateZ(view.group, model.get('z'), model.get('zlevel'));
};
+ function _updateZ(el: Element, z: number, zlevel: number) {
+ // Group may also have textContent
+ const label = el.getTextContent();
+ const labelLine = el.getTextGuideLine();
+ const isGroup = el.isGroup;
+
+ // always set z and zlevel if label/labelLine exists
+ if (label || labelLine) {
+ if (label) {
+ label.z = z + 1;
+ label.zlevel = zlevel;
+ // lift z2 of text content
+ // TODO if el.emphasis.z2 is spcefied, what about textContent.
+ isGroup || (label.z2 = (el as Displayable).z2 + 2);
+ }
+ if (labelLine) {
+ const showAbove = el.textGuideLineConfig && el.textGuideLineConfig.showAbove;
+ labelLine.z = z;
+ labelLine.zlevel = zlevel;
+ isGroup || (labelLine.z2 = (el as Displayable).z2 + (showAbove ? 1 : -1));
+ }
+ }
+
+ if (isGroup) {
+ // set z & zlevel of children elements of Group
+ el.traverse((childEl: Element) => _updateZ(childEl, z, zlevel));
Review comment:
Perhaps we can return max z2 of all elements in the group and update z2 of label and labelLine based on this max z2 value.
For example:
```ts
if (isGroup) {
const children = el.childrenRef();
let maxZ2 = -Infinity;
for (let i = 0; i < children.length; i++); {
maxZ2 =_updateZ(childEl, z, zlevel, maxZ2);
}
if (isFinite(maxZ2)) {
label.z2 = maxZ2 + 2;
labelLine.....
}
}
else {
return Math.max(el.z2, maxZ2);
}
```
Also this code may cause each group node to be traversed too many times because for each group node it will call `traverse` and visits all its descendants when it's visited. We need to loop the childrenRef() we want to do it recursively.
##########
File path: src/core/echarts.ts
##########
@@ -2157,34 +2157,44 @@ class ECharts extends Eventful<ECEventDefinition> {
if (model.preventAutoZ) {
return;
}
- const z = model.get('z');
- const zlevel = model.get('zlevel');
// Set z and zlevel
- view.group.traverse(function (el: Displayable) {
- if (!el.isGroup) {
- z != null && (el.z = z);
- zlevel != null && (el.zlevel = zlevel);
-
- // TODO if textContent is on group.
- const label = el.getTextContent();
- const labelLine = el.getTextGuideLine();
- if (label) {
- label.z = el.z;
- label.zlevel = el.zlevel;
- // lift z2 of text content
- // TODO if el.emphasis.z2 is spcefied, what about textContent.
- label.z2 = el.z2 + 2;
- }
- if (labelLine) {
- const showAbove = el.textGuideLineConfig && el.textGuideLineConfig.showAbove;
- labelLine.z = el.z;
- labelLine.zlevel = el.zlevel;
- labelLine.z2 = el.z2 + (showAbove ? 1 : -1);
- }
- }
- });
+ _updateZ(view.group, model.get('z'), model.get('zlevel'));
};
+ function _updateZ(el: Element, z: number, zlevel: number) {
+ // Group may also have textContent
+ const label = el.getTextContent();
+ const labelLine = el.getTextGuideLine();
+ const isGroup = el.isGroup;
+
+ // always set z and zlevel if label/labelLine exists
+ if (label || labelLine) {
+ if (label) {
+ label.z = z + 1;
Review comment:
I think `label.z` is better to be same with user-configured `z` so it can be more predictable to developers. Also, we hope elements can be on top of other labels when it's hovered, which can't be done if we lift the `z` by default.
--
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