-
Notifications
You must be signed in to change notification settings - Fork 19.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(colorBy): provide option.colorBy #13731 #13788
Conversation
Thanks for your contribution! The pull request is marked to be Document changes are required in this PR. Please also make a PR to apache/incubator-echarts-doc for document changes. When the doc PR is merged, the maintainers will remove the |
There is a questionWhat does the const colorBy = nodeModel.get('colorBy'); [ColorBy_AssignColorsTo_Children] [ColorBy_AssignColorsTo_ThisNode] But there might be some bad cases about [ColorBy_AssignColorsTo_ThisNode].[Question_1]option = {
series: {
data: [{
value: 100,
colorBy: 'value'
}, {
value: 200,
colorBy: 'value'
}, {
value: 300,
colorBy: 'id'
}]
}
}; This case will become complicated a bit under [ColorBy_AssignColorsTo_ThisNode] in implementation: [Question_2]If use [ColorBy_AssignColorsTo_Children]: option = {
series: {
levels: [{ // level0
itemStyle: { ... }
}, { // level1
color: ['#aaa', '#bbb', '#ccc'],
colorBy: 'value',
// By the upper config, all of the nodes in this level
// are mapped to ['#aaa', '#bbb', '#ccc']
itemStyle: {
gapWidth: 1
}
}, { // level2
color: ['blue', 'red', 'yellow', 'orange'],
colorBy: 'childIndex',
// By the upper config, all of the nodes in this level
// are mapped to ['blue', 'red', 'yellow', 'orange']
}]
}
}; But if use [ColorBy_AssignColorsTo_ThisNode] for the same effect: option = {
series: {
levels: [{ // level0
itemStyle: { ... }
}, { // level1
color: ['#aaa', '#bbb', '#ccc'],
itemStyle: {
gapWidth: 1
}
}, { // level2
// This config means that all nodes in this level will be mapped
// by 'value' to ['#aaa', '#bbb', '#ccc']
// rather than ['blue', 'red', 'yellow', 'orange'].
colorBy: 'value',
color: ['blue', 'red', 'yellow', 'orange'],
}, { // level3
// This config means that all nodes in this level will be mapped
// by 'childIndex' to ['blue', 'red', 'yellow', 'orange']
colorBy: 'childIndex'
}]
}
}; I am worried that it probably confuse users. [Question_3]See [Question_2], option = {
series: {
levels: [{ // level0
itemStyle: { ... }
}, { // level1
itemStyle: {
gapWidth: 1
}
}, { // level2
color: ['#aaa', '#bbb', '#ccc'],
colorBy: 'value',
// By the upper config, all of the nodes in this level
// are mapped to ['#aaa', '#bbb', '#ccc']
}, { // level3
color: ['blue', 'red', 'yellow', 'orange'],
colorBy: 'childIndex'
}]
}
}; But it brings another problem: option = {
series: {
data: [{
value: 100,
// I only want is node to map each child to
// ['#aaa', '#bbb', '#ccc'] by 'childIndex'
// If using [ColorBy_AssignColorsTo_ThisNode], we have to repeat
// the declaration of color and colorBy in each child:
children: [{
value: 11,
color: ['#aaa', '#bbb', '#ccc'],
colorBy: 'childIndex'
}, {
value: 21,
color: ['#aaa', '#bbb', '#ccc'],
colorBy: 'childIndex'
}, {
value: 31,
color: ['#aaa', '#bbb', '#ccc'],
colorBy: 'childIndex'
}
]
}, {
value: 200,
// I only want is node to map each child to
// ['blue', 'red', 'yellow', 'orange'] by 'value'
// If using [ColorBy_AssignColorsTo_ThisNode], we have to repeat
// the declaration of color and colorBy in each child:
children: [{
value: 991,
color: ['blue', 'red', 'yellow', 'orange'],
colorBy: 'value'
}, {
value: 992,
color: ['blue', 'red', 'yellow', 'orange'],
colorBy: 'value'
}
]
}]
}
}; But if use [ColorBy_AssignColorsTo_Children], we can simply: option = {
series: {
data: [{
value: 100,
// I only want is node to map each child to
// ['#aaa', '#bbb', '#ccc'] by 'childIndex'
color: ['#aaa', '#bbb', '#ccc'],
colorBy: 'childIndex',
children: [{
value: 11,
}, {
value: 21,
}, {
value: 31,
}
]
}, {
value: 200,
// I only want is node to map each child to
// ['blue', 'red', 'yellow', 'orange'] by 'value'
color: ['blue', 'red', 'yellow', 'orange'],
colorBy: 'value',
children: [{
value: 991,
}, {
value: 992,
}
]
}]
}
}; Finallyconsider the most common case: option = {
series: {
// We can see 100, 200, 333 as the child nodes of series,
// and series.colorBy can be understood as "the approach of
// assigning colors to the child nodes of series", which is
// compatible with [ColorBy_AssignColorsTo_Children]
colorBy: 'index',
data: [100, 200, 333]
}
} |
I would suggest using [ColorBy_AssignColorsTo_ThisNode] if my following statements stand. [Question_1]
Yes. It will be more complicated than [ColorBy_AssignColorsTo_Children]. But this way can provide the flexibility to use different colorBy policy for different children, e.g., child A wants to use policy X and child B wants to use policy Y. Would this be possible using [ColorBy_AssignColorsTo_Children]? [Question_2]Solve this problem using [Question_3]'s method. [Question_3]
option = {
series: {
data: [{
value: 100,
color: ['#aaa', '#bbb', '#ccc'],
// I only want is node to map each child to
// ['#aaa', '#bbb', '#ccc'] by 'childIndex'
// If using [ColorBy_AssignColorsTo_ThisNode], we have to repeat
// the declaration of color and colorBy in each child:
children: [{
value: 11,
colorBy: 'childIndex'
}, {
value: 21,
colorBy: 'childIndex'
}, {
value: 31,
colorBy: 'childIndex'
}
]
}, {
value: 200,
color: ['blue', 'red', 'yellow', 'orange'],
// I only want is node to map each child to
// ['blue', 'red', 'yellow', 'orange'] by 'value'
// If using [ColorBy_AssignColorsTo_ThisNode], we have to repeat
// the declaration of color and colorBy in each child:
children: [{
value: 991,
colorBy: 'value'
}, {
value: 992,
colorBy: 'value'
}
]
}]
}
}; Yes, ConclusionThe main reason I prefer [ColorBy_AssignColorsTo_ThisNode] better is that the idea to consider children was not universal among other options of ECharts. In most cases, we only consider the style for the item itself, and if children have special rules, it should be defined in children. I'm not strongly confident on this, but hope to provide some view of point on this. Thanks! |
[Question_1]
I think there is no such requirement in treemap. And it's too complicated. And this requirement (use different colorBy policy for different children) will bring complicated implementation when using
[Question_2]This confusion is still a problem: option = {
series: {
levels: [{ // level0
itemStyle: { ... }
}, { // level1
color: ['#aaa', '#bbb', '#ccc'],
itemStyle: {
gapWidth: 1
}
}, { // level2
// This config means that all nodes in this level will be mapped
// by 'value' to ['#aaa', '#bbb', '#ccc']
// rather than ['blue', 'red', 'yellow', 'orange'].
colorBy: 'value',
color: ['blue', 'red', 'yellow', 'orange'],
}, { // level3
// This config means that all nodes in this level will be mapped
// by 'childIndex' to ['blue', 'red', 'yellow', 'orange']
colorBy: 'childIndex'
}]
}
}; |
I'm not sure but will it be too complicated on design if we can have different |
@pissang At present, treemap But if changing the meaning "how to arrange the color of the node self", I have not found an easy way to implement it yet, even though only support it on |
I agree with this point. In this case, |
This PR does not change the behavior of treemap series but only the other series. |
Overall, it seems a single feature needs the same code snippet added in many places. So I don’t think it will scale well. Ideally, one change should be done in one place. |
src/visual/style.ts
Outdated
ecModel.eachSeries(function (seriesModel) { | ||
if (!seriesModel.useColorPaletteOnData || ecModel.isSeriesFiltered(seriesModel)) { | ||
ecModel.eachSeries((seriesModel: SeriesModel<SeriesOption & ColorByMixin>) => { | ||
const colorBy = seriesModel.getColorBy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original logic ecModel.isSeriesFiltered
seems to be removed. I think it's a mistake?
Congratulations! Your PR has been merged. Thanks for your contribution! 👍 |
Brief Information
This pull request is in the type of:
What does this PR do?
Provide an option for users to use color palette on each data items of the same series.
Fixed issues
Details
Before: What was the problem?
Series like bar charts use the color palette to distinguish different series. But there is no easy way to use the palette for each data item except than setting
itemStyle.color
for each data item, which is quite tedious.After: How is it fixed in this PR?
Series can set
colorBy
to be'item'
to use the color palette for each data item, which by default is'seriesId'
or other values depending on series type.Usage
Are there any API changes?
option.colorBy
andoption.series.colorBy
are provided, whose values can be:'series' | 'data'
.Related test cases or examples to use the new APIs
test/colorBy.html
With
Before:
After:
Others
Merging options
Other information
Todo:
series.colorBy
for each series typecolorMappingBy
and usecolorBy
to compatEnable sunburst using different color palette policies like using the palette for each level or each data itemThis PR does not change the behavior of treemap or sunburst series but only the other series.
Treemap and sunburst behavior on color mapping should be discussed and implemented in future PRs.