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/10/10 12:57:37 UTC
[GitHub] [echarts] susiwen8 opened a new pull request #15851: fix(label): label not show on singleaxis
susiwen8 opened a new pull request #15851:
URL: https://github.com/apache/echarts/pull/15851
<!-- 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?
For `singleaxis`, it doesn't have `defaultedLabel`, then use `value` as default dim to retrieve label.
### Fixed issues
Close #15830
## Details
### Before: What was the problem?
<img width="676" alt="Screen Shot 2021-10-10 at 8 42 41 PM" src="https://user-images.githubusercontent.com/20318608/136696200-f4f48892-a0d4-45a6-87a7-f302e676aa4b.png">
### After: How is it fixed in this PR?
<img width="681" alt="Screen Shot 2021-10-10 at 8 42 28 PM" src="https://user-images.githubusercontent.com/20318608/136696204-fae5c442-db6f-4a53-99f6-7170aebc4f5e.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
`test/scatter-single-axis.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.
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] 100pah commented on a change in pull request #15851: fix(label): label not show on singleaxis
Posted by GitBox <gi...@apache.org>.
100pah commented on a change in pull request #15851:
URL: https://github.com/apache/echarts/pull/15851#discussion_r765423673
##########
File path: src/data/helper/dimensionHelper.ts
##########
@@ -169,6 +173,10 @@ export function summarizeDimensions(
defaultedLabel = encodeLabel.slice();
}
+ if (!defaultedLabel || !defaultedLabel.length) {
+ defaultedLabel = [lastExtraCoord];
Review comment:
Should better not give a defaultLabel as `['']`, (the `lastExtraCoord` is initialized with `''`, and there is no non-empty judgement.
##########
File path: src/data/helper/dimensionHelper.ts
##########
@@ -130,6 +131,9 @@ export function summarizeDimensions(
getOrCreateEncodeArr(userOutputEncode, coordDim)[coordDimIndex] =
data.getDimensionIndex(dimItem.name);
}
+ else {
+ lastExtraCoord = dimName;
Review comment:
The [strategy A](https://github.com/apache/echarts/pull/15851/commits/ddd5f9e113bbb6ec6ec20d5ad9545fba24184d25#r729775267) means that:
> (A) if no defaultedLabel selected, use the last !isExtraCoord dimension no matter it's category or time
That is, find the last coord that is "not extra coord".
But here the code finds the last coord that is "extra coord".
##########
File path: src/data/helper/dimensionHelper.ts
##########
@@ -169,6 +173,10 @@ export function summarizeDimensions(
defaultedLabel = encodeLabel.slice();
}
+ if (!defaultedLabel || !defaultedLabel.length) {
Review comment:
The `!defaultedLabel` will always be `false`.
Only `if (!defaultedLabel.length) {...}` is enough
--
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] susiwen8 commented on a change in pull request #15851: fix(label): label not show on singleaxis
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #15851:
URL: https://github.com/apache/echarts/pull/15851#discussion_r727185871
##########
File path: src/chart/helper/labelHelper.ts
##########
@@ -45,6 +45,10 @@ export function getDefaultLabel(
}
return vals.join(' ');
}
+ else {
+ const rawVal = retrieveRawValue(data, dataIndex, 'value');
Review comment:
For `singleAxis`, the dimension are `['single', 'value']` when data format is an array. Normally, in [summarizeDimensions](https://github.com/apache/echarts/blob/master/src/data/helper/dimensionHelper.ts#L117) it will assign `value` as `defaultLabel`
https://github.com/apache/echarts/blob/master/src/data/helper/dimensionHelper.ts#L125
but in [prepareSeriesDataSchema](
https://github.com/apache/echarts/blob/master/src/data/helper/createDimensions.ts#L267) it marks `value` as `isExtraCoord` so `singleAxis` would skip this logic. That cause `defaultLabel` is an empty array. I was trying to add
```js
if (defaultedLabel && defaultedLabel.length) {
defaultedLabel = encode.value.slice();
}
```
However `value` wasn't always in `DimensionSummary`, also, if data format weren't `array`, which it is allowed. The dimension for `singleAxis` would be `['single']`. @pissang
--
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] susiwen8 commented on a change in pull request #15851: fix(label): label not show on singleaxis
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #15851:
URL: https://github.com/apache/echarts/pull/15851#discussion_r727185871
##########
File path: src/chart/helper/labelHelper.ts
##########
@@ -45,6 +45,10 @@ export function getDefaultLabel(
}
return vals.join(' ');
}
+ else {
+ const rawVal = retrieveRawValue(data, dataIndex, 'value');
Review comment:
For `singleAxis`, the dimension are `['single', 'value']` when data format is an array. Normally, in [summarizeDimensions](https://github.com/apache/echarts/blob/master/src/data/helper/dimensionHelper.ts#L117) it will assign `value` as `defaultLabel`
https://github.com/apache/echarts/blob/master/src/data/helper/dimensionHelper.ts#L125
but in [prepareSeriesDataSchema](
https://github.com/apache/echarts/blob/master/src/data/helper/createDimensions.ts#L267) it marks `value` as `isExtraCoord` so `singleAxis` would skip this logic. That cause `defaultLabel` is an empty array. I was trying to add
```js
if (defaultedLabel && defaultedLabel.length) {
defaultedLabel = encode.value.slice();
}
```
However `value` wasn't always in `DimensionSummary`, also, if data format weren't `array`, which it is allowed. The dimension for `singleAxis` would be `['single']`.
--
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] 100pah commented on a change in pull request #15851: fix(label): label not show on singleaxis
Posted by GitBox <gi...@apache.org>.
100pah commented on a change in pull request #15851:
URL: https://github.com/apache/echarts/pull/15851#discussion_r729775267
##########
File path: src/chart/helper/labelHelper.ts
##########
@@ -45,6 +45,10 @@ export function getDefaultLabel(
}
return vals.join(' ');
}
+ else {
+ const rawVal = retrieveRawValue(data, dataIndex, 'value');
Review comment:
@susiwen8 I think it's probably not a good strategy to `retrieveRawValue(data, dataIndex, 'value');` here.
The `option` provided in #15830 is:
```js
option = {
singleAxis: {
type: 'category',
data: ['a', 'b', 'c']
},
series: {
data: [
value: [0, 5]
]
}
}
```
where only the first dimension of data (that is, `0`) is used by the coord sys,
and the second dimension (that is, `5`) is not used by the coord sys.
Consider the data might comes form `series.data` or `dataset.source`,
`!isExtraCoord` means: this is a dimension that used by this series and used by coord sys.
At present the strategy that "find a proper dimension as the default label" is:
only find from the dimensions that are `!isExtraCoord`.
Because if data comes from dataset, `isExtraCoord` dimensions may be used by other series.
But there is another detail in the strategy: it only choose dimensions that are not of type `category` and `time`.
That is why the label did not display in case #15830 , where the only `!isExtraCoord` dimension is a `category` dimension. See https://github.com/apache/echarts/blob/master/src/data/helper/dimensionHelper.ts#L124
I think we could change the strategy in https://github.com/apache/echarts/blob/master/src/data/helper/dimensionHelper.ts#L124 to:
(A) if no `defaultedLabel` selected, use the last `!isExtraCoord` dimension no matter it's `category` or `time`.
(B) if data is from series rather than dataset, do not need to consider `isExtraCoord` when finding `defaultedLabel`.
(A) can solve #15830
(B) might make it better but brings a breaking change.
--
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 #15851: fix(label): label not show on singleaxis
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #15851:
URL: https://github.com/apache/echarts/pull/15851#discussion_r725746870
##########
File path: src/chart/helper/labelHelper.ts
##########
@@ -45,6 +45,10 @@ export function getDefaultLabel(
}
return vals.join(' ');
}
+ else {
+ const rawVal = retrieveRawValue(data, dataIndex, 'value');
Review comment:
This branch can be merged with the first branch to make code tighter. Also, I think we should figure out why `singleAxis` coordinate system doesn't have the `defaultLabel`. It's an unexpected behavior
--
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 #15851: fix(label): label not show on singleaxis
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #15851:
URL: https://github.com/apache/echarts/pull/15851#discussion_r725746870
##########
File path: src/chart/helper/labelHelper.ts
##########
@@ -45,6 +45,10 @@ export function getDefaultLabel(
}
return vals.join(' ');
}
+ else {
+ const rawVal = retrieveRawValue(data, dataIndex, 'value');
Review comment:
This branch can be merged with the first branch to have tighter code. Also, I think we should figure out why `singleAxis` coordinate system doesn't have the `defaultLabel`. It's an unexpected behavior
--
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] susiwen8 commented on a change in pull request #15851: fix(label): label not show on singleaxis
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #15851:
URL: https://github.com/apache/echarts/pull/15851#discussion_r732924011
##########
File path: src/chart/helper/labelHelper.ts
##########
@@ -45,6 +45,10 @@ export function getDefaultLabel(
}
return vals.join(' ');
}
+ else {
+ const rawVal = retrieveRawValue(data, dataIndex, 'value');
Review comment:
I'm incline solution A
--
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 #15851: fix(label): label not show on singleaxis
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #15851:
URL: https://github.com/apache/echarts/pull/15851#issuecomment-939477921
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 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