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/11/09 10:23:23 UTC
[GitHub] [echarts] Ovilia opened a new pull request #16034: Fix pie label position with rich text
Ovilia opened a new pull request #16034:
URL: https://github.com/apache/echarts/pull/16034
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
<!-- USE ONCE SENTENCE TO DESCRIBE WHAT THIS PR DOES. -->
Fix pie label position with rich text.
### Fixed issues
#16023
## Details
### Before: What was the problem?
### After: How is it fixed in this PR?
Labels should not be outside of the canvas and positions of background and labelLine are fixed.
## Misc
<!-- ADD RELATED ISSUE ID WHEN APPLICABLE -->
- [ ] The API has been changed (apache/echarts-doc#xxx).
- [x] This PR depends on ZRender changes (ecomfe/zrender#847).
### Related test cases or examples to use the new APIs
test/pie-label.html
## Others
### Merging options
- [ ] Please squash the commits into a single one when merge.
### Other information
- [ ] Run visual test of pie charts.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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] [echarts] pissang commented on a change in pull request #16034: fix(pie): label layout and text wrapping
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16034:
URL: https://github.com/apache/echarts/pull/16034#discussion_r764019803
##########
File path: src/chart/pie/labelLayout.ts
##########
@@ -228,6 +255,91 @@ function avoidOverlap(
}
}
+/**
+ * Set max width of each label, and then wrap each label to the max width.
+ *
+ * @param layout label layout
+ * @param availableWidth max width for the label to display
+ * @param forceRecalculate recaculate the text layout even if the current width
+ * is smaller than `availableWidth`. This is useful when the text was previously
+ * wrapped by calling `layoutText` but now `availableWidth` changed, in which
+ * case, previous wrapping should be redo. `forceRecalculate` is ignored if
+ * `labelStyleWidth` is set.
+ */
+function layoutText(
Review comment:
The method name should be more specific and focused on the thing it does.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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] [echarts] echarts-bot[bot] commented on pull request #16034: fix(pie): label layout and text wrapping
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #16034:
URL: https://github.com/apache/echarts/pull/16034#issuecomment-998460532
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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] [echarts] Ovilia commented on pull request #16034: fix(pie): label layout and text wrapping
Posted by GitBox <gi...@apache.org>.
Ovilia commented on pull request #16034:
URL: https://github.com/apache/echarts/pull/16034#issuecomment-988467744
@pissang Thanks for your review. I've made a new commit to fix them and no new diff in this commit. Please review again when you have time.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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] [echarts] Ovilia edited a comment on pull request #16034: fix(pie): label layout and text wrapping
Posted by GitBox <gi...@apache.org>.
Ovilia edited a comment on pull request #16034:
URL: https://github.com/apache/echarts/pull/16034#issuecomment-979845808
TODO:
- [x] Explain the diff of visual test result if necessary.
- [x] Code style cleaning.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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] [echarts] Ovilia edited a comment on pull request #16034: fix(pie): label layout and text wrapping
Posted by GitBox <gi...@apache.org>.
Ovilia edited a comment on pull request #16034:
URL: https://github.com/apache/echarts/pull/16034#issuecomment-979845808
TODO:
- [ ] Explain the diff of visual test result if necessary.
- [x] Code style cleaning.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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] [echarts] pissang commented on a change in pull request #16034: fix(pie): label layout and text wrapping
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16034:
URL: https://github.com/apache/echarts/pull/16034#discussion_r764024437
##########
File path: src/chart/pie/labelLayout.ts
##########
@@ -228,6 +255,91 @@ function avoidOverlap(
}
}
+/**
+ * Set max width of each label, and then wrap each label to the max width.
+ *
+ * @param layout label layout
+ * @param availableWidth max width for the label to display
+ * @param forceRecalculate recaculate the text layout even if the current width
+ * is smaller than `availableWidth`. This is useful when the text was previously
+ * wrapped by calling `layoutText` but now `availableWidth` changed, in which
+ * case, previous wrapping should be redo. `forceRecalculate` is ignored if
+ * `labelStyleWidth` is set.
+ */
+function layoutText(
+ layout: LabelLayout,
+ availableWidth: number,
+ forceRecalculate: boolean = false
+) {
+ const label = layout.label;
+ const style = label.style;
+ const textRect = layout.rect;
+ const bgColor = style.backgroundColor;
+ const padding = style.padding as number[];
+ const paddingH = padding ? padding[1] + padding[3] : 0;
+ const overflow = style.overflow;
+
+ // textRect.width already contains paddingH if bgColor is set
+ const oldOuterWidth = textRect.width + (bgColor ? 0 : paddingH);
+ if (availableWidth < oldOuterWidth
+ || forceRecalculate && layout.labelStyleWidth == null
+ ) {
+ const oldHeight = textRect.height;
+ if (overflow && overflow.match('break')) {
+ const oldEllipsis = style.ellipsis;
+
+ // Temporarily set background to be null to calculate
+ // the bounding box without backgroud.
+ label.setStyle('backgroundColor', null);
+ label.setStyle('ellipsis', '');
+
+ // Set constraining width
+ label.setStyle('width', availableWidth - paddingH);
+
+ if (layout.labelStyleWidth == null) {
+ // This is the real bounding box of the text without padding
+ const innerRect = label.getBoundingRect();
+ innerRect.applyTransform(label.getComputedTransform());
Review comment:
There is no need to applyTransform because only scale transform will affect the width, which doesn't exits in pie labels
##########
File path: src/chart/pie/labelLayout.ts
##########
@@ -228,6 +255,91 @@ function avoidOverlap(
}
}
+/**
+ * Set max width of each label, and then wrap each label to the max width.
+ *
+ * @param layout label layout
+ * @param availableWidth max width for the label to display
+ * @param forceRecalculate recaculate the text layout even if the current width
+ * is smaller than `availableWidth`. This is useful when the text was previously
+ * wrapped by calling `layoutText` but now `availableWidth` changed, in which
+ * case, previous wrapping should be redo. `forceRecalculate` is ignored if
+ * `labelStyleWidth` is set.
+ */
+function layoutText(
+ layout: LabelLayout,
+ availableWidth: number,
+ forceRecalculate: boolean = false
+) {
+ const label = layout.label;
+ const style = label.style;
+ const textRect = layout.rect;
+ const bgColor = style.backgroundColor;
+ const padding = style.padding as number[];
+ const paddingH = padding ? padding[1] + padding[3] : 0;
+ const overflow = style.overflow;
+
+ // textRect.width already contains paddingH if bgColor is set
+ const oldOuterWidth = textRect.width + (bgColor ? 0 : paddingH);
+ if (availableWidth < oldOuterWidth
+ || forceRecalculate && layout.labelStyleWidth == null
+ ) {
+ const oldHeight = textRect.height;
+ if (overflow && overflow.match('break')) {
+ const oldEllipsis = style.ellipsis;
+
+ // Temporarily set background to be null to calculate
+ // the bounding box without backgroud.
+ label.setStyle('backgroundColor', null);
+ label.setStyle('ellipsis', '');
+
+ // Set constraining width
+ label.setStyle('width', availableWidth - paddingH);
+
+ if (layout.labelStyleWidth == null) {
+ // This is the real bounding box of the text without padding
+ const innerRect = label.getBoundingRect();
+ innerRect.applyTransform(label.getComputedTransform());
+ label.setStyle('width', Math.ceil(innerRect.width));
+ }
+
+ label.setStyle('backgroundColor', bgColor);
+ label.setStyle('ellipsis', oldEllipsis);
+ }
+ else {
+ const availableInnerWidth = availableWidth - paddingH;
+ if (availableWidth < oldOuterWidth) {
+ // Current text is too wide, use `availableWidth` as max width.
+ label.setStyle('width', availableInnerWidth);
Review comment:
To make the code size smaller. We can use a temporary variable `newWidth`
```ts
newWidth = availableInnerWidth;
```
Then we set this `newWidth` to style at last.
```ts
label.setStyle('width', newWidth);
```
##########
File path: src/chart/pie/labelLayout.ts
##########
@@ -228,6 +255,91 @@ function avoidOverlap(
}
}
+/**
+ * Set max width of each label, and then wrap each label to the max width.
+ *
+ * @param layout label layout
+ * @param availableWidth max width for the label to display
+ * @param forceRecalculate recaculate the text layout even if the current width
+ * is smaller than `availableWidth`. This is useful when the text was previously
+ * wrapped by calling `layoutText` but now `availableWidth` changed, in which
+ * case, previous wrapping should be redo. `forceRecalculate` is ignored if
+ * `labelStyleWidth` is set.
+ */
+function layoutText(
Review comment:
The method name should be more clear and focused on the thing it does.
##########
File path: src/chart/pie/labelLayout.ts
##########
@@ -228,6 +255,91 @@ function avoidOverlap(
}
}
+/**
+ * Set max width of each label, and then wrap each label to the max width.
+ *
+ * @param layout label layout
+ * @param availableWidth max width for the label to display
+ * @param forceRecalculate recaculate the text layout even if the current width
+ * is smaller than `availableWidth`. This is useful when the text was previously
+ * wrapped by calling `layoutText` but now `availableWidth` changed, in which
+ * case, previous wrapping should be redo. `forceRecalculate` is ignored if
+ * `labelStyleWidth` is set.
+ */
+function layoutText(
+ layout: LabelLayout,
+ availableWidth: number,
+ forceRecalculate: boolean = false
+) {
+ const label = layout.label;
+ const style = label.style;
+ const textRect = layout.rect;
+ const bgColor = style.backgroundColor;
+ const padding = style.padding as number[];
+ const paddingH = padding ? padding[1] + padding[3] : 0;
+ const overflow = style.overflow;
+
+ // textRect.width already contains paddingH if bgColor is set
+ const oldOuterWidth = textRect.width + (bgColor ? 0 : paddingH);
+ if (availableWidth < oldOuterWidth
+ || forceRecalculate && layout.labelStyleWidth == null
+ ) {
+ const oldHeight = textRect.height;
+ if (overflow && overflow.match('break')) {
+ const oldEllipsis = style.ellipsis;
+
+ // Temporarily set background to be null to calculate
+ // the bounding box without backgroud.
+ label.setStyle('backgroundColor', null);
+ label.setStyle('ellipsis', '');
Review comment:
There is no need to set ellipsis because only `overflow: break` will enter this branch
##########
File path: src/chart/pie/labelLayout.ts
##########
@@ -228,6 +255,91 @@ function avoidOverlap(
}
}
+/**
+ * Set max width of each label, and then wrap each label to the max width.
+ *
+ * @param layout label layout
+ * @param availableWidth max width for the label to display
+ * @param forceRecalculate recaculate the text layout even if the current width
+ * is smaller than `availableWidth`. This is useful when the text was previously
+ * wrapped by calling `layoutText` but now `availableWidth` changed, in which
+ * case, previous wrapping should be redo. `forceRecalculate` is ignored if
+ * `labelStyleWidth` is set.
+ */
+function layoutText(
+ layout: LabelLayout,
+ availableWidth: number,
+ forceRecalculate: boolean = false
+) {
+ const label = layout.label;
+ const style = label.style;
+ const textRect = layout.rect;
+ const bgColor = style.backgroundColor;
+ const padding = style.padding as number[];
+ const paddingH = padding ? padding[1] + padding[3] : 0;
+ const overflow = style.overflow;
+
+ // textRect.width already contains paddingH if bgColor is set
+ const oldOuterWidth = textRect.width + (bgColor ? 0 : paddingH);
+ if (availableWidth < oldOuterWidth
+ || forceRecalculate && layout.labelStyleWidth == null
Review comment:
I think it's not necessary to enter this whole method if `labelStyleWidth` is not null
##########
File path: src/chart/pie/labelLayout.ts
##########
@@ -228,6 +255,91 @@ function avoidOverlap(
}
}
+/**
+ * Set max width of each label, and then wrap each label to the max width.
+ *
+ * @param layout label layout
+ * @param availableWidth max width for the label to display
+ * @param forceRecalculate recaculate the text layout even if the current width
+ * is smaller than `availableWidth`. This is useful when the text was previously
+ * wrapped by calling `layoutText` but now `availableWidth` changed, in which
+ * case, previous wrapping should be redo. `forceRecalculate` is ignored if
+ * `labelStyleWidth` is set.
+ */
+function layoutText(
+ layout: LabelLayout,
+ availableWidth: number,
+ forceRecalculate: boolean = false
+) {
+ const label = layout.label;
+ const style = label.style;
+ const textRect = layout.rect;
+ const bgColor = style.backgroundColor;
+ const padding = style.padding as number[];
+ const paddingH = padding ? padding[1] + padding[3] : 0;
+ const overflow = style.overflow;
+
+ // textRect.width already contains paddingH if bgColor is set
+ const oldOuterWidth = textRect.width + (bgColor ? 0 : paddingH);
+ if (availableWidth < oldOuterWidth
+ || forceRecalculate && layout.labelStyleWidth == null
+ ) {
+ const oldHeight = textRect.height;
+ if (overflow && overflow.match('break')) {
+ const oldEllipsis = style.ellipsis;
+
+ // Temporarily set background to be null to calculate
+ // the bounding box without backgroud.
+ label.setStyle('backgroundColor', null);
+ label.setStyle('ellipsis', '');
+
+ // Set constraining width
+ label.setStyle('width', availableWidth - paddingH);
+
+ if (layout.labelStyleWidth == null) {
+ // This is the real bounding box of the text without padding
+ const innerRect = label.getBoundingRect();
+ innerRect.applyTransform(label.getComputedTransform());
+ label.setStyle('width', Math.ceil(innerRect.width));
+ }
+
+ label.setStyle('backgroundColor', bgColor);
+ label.setStyle('ellipsis', oldEllipsis);
+ }
+ else {
+ const availableInnerWidth = availableWidth - paddingH;
+ if (availableWidth < oldOuterWidth) {
+ // Current text is too wide, use `availableWidth` as max width.
+ label.setStyle('width', availableInnerWidth);
Review comment:
To make the code size smaller. We can use a temporary variable `newWidth`
```ts
newWidth = availableInnerWidth;
```
Then we set this `newWidth` to style at last.
```ts
label.setStyle('width', newWidth);
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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] [echarts] pissang merged pull request #16034: fix(pie): label layout and text wrapping
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #16034:
URL: https://github.com/apache/echarts/pull/16034
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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] [echarts] echarts-bot[bot] commented on pull request #16034: Fix pie label position with rich text
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #16034:
URL: https://github.com/apache/echarts/pull/16034#issuecomment-964012120
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/echarts/wiki/How-to-make-a-pull-request).
The pull request is marked to be `PR: author is committer` because you are a committer of this project.
This PR depends on [ZRender](https://github.com/ecomfe/zrender) changes. Please update the ZRender dependency to the latest nightly version including this change, which takes place everyday at 8:00 UTC (16:00 Beijing Time).
You can use `npm i zrender@npm:zrender-nightly` to update package.json.
If you have any question about this, please leave a comment and we will give you extra help on this.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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] [echarts] Ovilia commented on pull request #16034: fix(pie): label layout and text wrapping
Posted by GitBox <gi...@apache.org>.
Ovilia commented on pull request #16034:
URL: https://github.com/apache/echarts/pull/16034#issuecomment-979845808
TODO:
- [ ] Explain the diff of visual test result if necessary.
- [ ] Code style cleaning.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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