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/08/23 10:33:38 UTC
[GitHub] [echarts] svedova opened a new pull request #15583: Fix: time axis overlapped labels
svedova opened a new pull request #15583:
URL: https://github.com/apache/echarts/pull/15583
<!-- 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. -->
This PR fixes the overlapping labels in the Time Axis.
### Fixed issues
<!--
- #xxxx: ...
-->
- https://github.com/apache/echarts/issues/14266
## Details
| Before | After |
| ------- | ----- |
| <img width="600" alt="Screenshot 2021-08-23 at 13 22 15" src="https://user-images.githubusercontent.com/3321893/130433229-1d947bfe-57b3-40cb-8160-807d2a3db1e7.png"> | <img width="600" alt="Screenshot 2021-08-23 at 13 21 54" src="https://user-images.githubusercontent.com/3321893/130433254-1e44f5f0-98a1-4583-8733-e4b3abcd075c.png"> |
## Misc
<!-- ADD RELATED ISSUE ID WHEN APPLICABLE -->
- [ ] The API has been changed (apache/echarts-doc#xxx).
- [ ] This PR depends on ZRender changes (ecomfe/zrender#xxx).
### Related test cases or examples to use the new APIs
NA.
## Others
### Merging options
- [x] 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.
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] svedova commented on pull request #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
svedova commented on pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#issuecomment-906259900
@pissang applied the changes, thanks for your help 🏓
--
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 #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#discussion_r694552205
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -347,6 +348,16 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
fixMinMaxLabelShow(axisModel, labelEls, ticksEls);
buildAxisMinorTicks(group, transformGroup, axisModel, opt.tickDirection);
+
+ const labelList = prepareLayoutList(labelEls.map(label => ({
+ label,
+ priority: label.style.text?.includes('primary') ? 1 : 0,
Review comment:
`getTicks` method of `Time.ts` has a `level` property. It can be used as priority. And `labelEls` is created from the list of `getTicks`.
--
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] svedova commented on pull request #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
svedova commented on pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#issuecomment-904640777
@pissang great idea! I've applied the suggested changes and here's the screenshot that shows the chart is still working as expected:
I'm going to ask for another round of review 🙏
--
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 edited a comment on pull request #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#issuecomment-904368882
@svedova Thanks! This change looks good to me and the result looks great! Perhaps we can limit this hiding overlap strategy to the time axis currently.
--
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 #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#discussion_r695368592
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -347,6 +348,20 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
fixMinMaxLabelShow(axisModel, labelEls, ticksEls);
buildAxisMinorTicks(group, transformGroup, axisModel, opt.tickDirection);
+
+ // This bit fixes the label overlap issue for the Time chart.
+ // See https://github.com/apache/echarts/issues/14266 for more.
+ if (axisModel.type === 'xAxis.time') {
+ const labelList = prepareLayoutList(labelEls.map(label => ({
+ label,
+ priority: label.zlevel,
Review comment:
As mentioned above. We can use `z2` value as a priority.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -809,6 +824,8 @@ function buildAxisLabel(
transformGroup.add(textEl);
textEl.updateTransform();
+ textEl.zlevel = labelItem.level;
Review comment:
`zlevel` is configured by developers. Each `zlevel` represents an individual canvas. Instead, we can use `z2` to set the painting order of each element inside each component. So perhaps we can add this level to the `z2` in https://github.com/apache/echarts/blob/7d33cfb062d7035c5c3bce61bcb5a269d0762daf/src/component/axis/AxisBuilder.ts#L784
Another problem seems only `time` axis has this `level` property. We can either check if `level` exists or use a default `0` value.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -347,6 +348,20 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
fixMinMaxLabelShow(axisModel, labelEls, ticksEls);
buildAxisMinorTicks(group, transformGroup, axisModel, opt.tickDirection);
+
+ // This bit fixes the label overlap issue for the Time chart.
+ // See https://github.com/apache/echarts/issues/14266 for more.
+ if (axisModel.type === 'xAxis.time') {
Review comment:
It will be slightly better to do the check by `axisModel.get('type') === 'time'`. Because this axis may be `yAxis` if the layout is vertical.
--
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] svedova commented on a change in pull request #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
svedova commented on a change in pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#discussion_r693857203
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -347,6 +348,16 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
fixMinMaxLabelShow(axisModel, labelEls, ticksEls);
buildAxisMinorTicks(group, transformGroup, axisModel, opt.tickDirection);
+
+ const labelList = prepareLayoutList(labelEls.map(label => ({
+ label,
+ priority: label.style.text?.includes('primary') ? 1 : 0,
Review comment:
@pissang I was not able to understand how to generate the priorities here so I did something very dirty to see if this works or not. Seems to be working but what is the proper way to assign the priority to the label? I think this should be done somewhere in https://github.com/apache/echarts/blob/master/src/scale/Time.ts as this method is used by many charts, if not all.
--
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 #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #15583:
URL: https://github.com/apache/echarts/pull/15583
--
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] svedova commented on pull request #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
svedova commented on pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#issuecomment-905752026
@pissang Thanks a lot for the second round review! Your help is hugely appreciated. I've submitted another commit with the suggested changes, so now back to 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.
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 #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#issuecomment-903646431
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).
--
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 #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#discussion_r696300123
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -766,7 +781,7 @@ function buildAxisLabel(
y: opt.labelOffset + opt.labelDirection * labelMargin,
rotation: labelLayout.rotation,
silent: silent,
- z2: 10,
+ z2: labelItem.level,
Review comment:
Should consider the base `10`. Or it may be under the axis line and tick lines. Another issue is `labelItem.level` may be `undefined` if it's not from the time axis. We should fallback to 0 in this case. For example:
```ts
z2: 10 + (labelItem.level || 0)
```
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -347,6 +348,20 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
fixMinMaxLabelShow(axisModel, labelEls, ticksEls);
buildAxisMinorTicks(group, transformGroup, axisModel, opt.tickDirection);
+
+ // This bit fixes the label overlap issue for the Time chart.
+ // See https://github.com/apache/echarts/issues/14266 for more.
+ if (axisModel.get('type') === 'time') {
+ const labelList = prepareLayoutList(labelEls.map(label => ({
Review comment:
Using `map` method from `zrender/src/core/util` will be slightly better in practice.
```ts
prepareLayoutList(map(labelEls, (label) => ...));
```
For the old times, using our map method can handle the incompatibility problem in ancient browsers. But for now, I think it's just for consistency. Perhaps we will remove all these ancient codes in the further times.
--
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] svedova commented on pull request #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
svedova commented on pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#issuecomment-903652734
@pissang can you please review this MR and tell me if it's in the right direction?
--
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 pull request #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#issuecomment-904368882
@svedova Thanks! The change looks good to me. Perhaps we can limit this hiding overlap strategy to the time axis currently.
--
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 #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#discussion_r695368592
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -347,6 +348,20 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
fixMinMaxLabelShow(axisModel, labelEls, ticksEls);
buildAxisMinorTicks(group, transformGroup, axisModel, opt.tickDirection);
+
+ // This bit fixes the label overlap issue for the Time chart.
+ // See https://github.com/apache/echarts/issues/14266 for more.
+ if (axisModel.type === 'xAxis.time') {
+ const labelList = prepareLayoutList(labelEls.map(label => ({
+ label,
+ priority: label.zlevel,
Review comment:
As mentioned above. We can use the`z2` value as priority.
--
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 #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#issuecomment-911039897
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] svedova commented on a change in pull request #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
svedova commented on a change in pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#discussion_r696474185
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -347,6 +348,20 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
fixMinMaxLabelShow(axisModel, labelEls, ticksEls);
buildAxisMinorTicks(group, transformGroup, axisModel, opt.tickDirection);
+
+ // This bit fixes the label overlap issue for the Time chart.
+ // See https://github.com/apache/echarts/issues/14266 for more.
+ if (axisModel.get('type') === 'time') {
+ const labelList = prepareLayoutList(labelEls.map(label => ({
Review comment:
Done as well 🙏
--
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] svedova commented on a change in pull request #15583: Fix: time axis overlapped labels
Posted by GitBox <gi...@apache.org>.
svedova commented on a change in pull request #15583:
URL: https://github.com/apache/echarts/pull/15583#discussion_r696474001
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -766,7 +781,7 @@ function buildAxisLabel(
y: opt.labelOffset + opt.labelDirection * labelMargin,
rotation: labelLayout.rotation,
silent: silent,
- z2: 10,
+ z2: labelItem.level,
Review comment:
Done!
--
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