Skip to content
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

Merged
merged 21 commits into from Jul 20, 2021
Merged

feat(colorBy): provide option.colorBy #13731 #13788

merged 21 commits into from Jul 20, 2021

Conversation

Ovilia
Copy link
Contributor

@Ovilia Ovilia commented Dec 10, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

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?

  • The API has been changed.

option.colorBy and option.series.colorBy are provided, whose values can be: 'series' | 'data'.

Related test cases or examples to use the new APIs

test/colorBy.html

With

option = {
    xAxis: {
        type: 'category',
        data: ['Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat', 'Sun']
    },
    yAxis: {
        type: 'value'
    },
    series: [{
        data: [120, 200, 150, 80, 70, 110, 130],
        type: 'bar',
        colorBy: 'data'
    }]
};

Before:

image

After:

image

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

Todo:

  • Provide series.colorBy for each series type
  • Deprecate colorMappingBy and use colorBy to compat
  • Enable sunburst using different color palette policies like using the palette for each level or each data item

This 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.

@echarts-bot
Copy link

echarts-bot bot commented Dec 10, 2020

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about 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.

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 PR: awaiting doc label.

src/chart/pie/PieSeries.ts Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 5, 2021
@100pah
Copy link
Member

100pah commented Feb 17, 2021

@Ovilia @pissang

There is a question

What does the colorBy mean exactly?

const colorBy = nodeModel.get('colorBy');

[ColorBy_AssignColorsTo_Children]
The colorBy is used to assign the color to the children of this node.
That is, colorBy: 'index' means that each child will be colored by their dataIndex.

[ColorBy_AssignColorsTo_ThisNode]
The colorBy is used to assign the color to this node itself.
That is, colorBy: 'index' means that this node will be colored by its dataIndex.
(The implementation of this PR use this understanding in sunburst at present).

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:
Before we calculate the colors we need to do the extra work that find all of the child nodes with colorBy: 'value'.
And the other children with colorBy: 'childIndex' | 'id' | ... will needs other treatment.
But [ColorBy_AssignColorsTo_Children] make it simple: the color rule is defined on the parent node.

[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],
we can also alter the definition to make it look better like is:

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:
consider this case:

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, 
               }
            ]

         }]  
     }
};

Finally

consider 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]  
     }
}

@Ovilia
Copy link
Contributor Author

Ovilia commented Feb 19, 2021

I would suggest using [ColorBy_AssignColorsTo_ThisNode] if my following statements stand.

[Question_1]

This case will become complicated a bit under [ColorBy_AssignColorsTo_ThisNode] in implementation

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]

color: ['#aaa', '#bbb', '#ccc'] doesn't need to be repeated in children because color is inherited with children. It should be:

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, colorBy: 'childIndex' still need to be written for multiple times, which is mainly because the node with value 100 and 200 have different colorBy policy among their children. This is acceptable just like in other cases we may also include the same itemStyle in the data for multiple times when some of the data use the same style while some of them not.

Conclusion

The 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!

@100pah
Copy link
Member

100pah commented Feb 19, 2021

@Ovilia

[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]?

I think there is no such requirement in treemap. And it's too complicated.
And [ColorBy_AssignColorsTo_Children] can also achieve this effect:
If a node need special color, use itemStyle is a more intuitive way.

And this requirement (use different colorBy policy for different children) will bring complicated implementation when using colorBy: 'value', where we need collect children with colorBy: 'value'.

## [Question_3]

color: ['#aaa', '#bbb', '#ccc'] doesn't need to be repeated in children because color is inherited with children.

The calculated color will be passed to children, but the color array on option can not be inherited by children.
For any node, nodeMode.get('color') can only get color from:
data item or levels[i] or series.

[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'
        }]
    }
};

@pissang
Copy link
Contributor

pissang commented Mar 1, 2021

I'm not sure but will it be too complicated on design if we can have different colorBy on each node. Will it be much simpler and less confusing with the itemStyle.color if we only support colorBy on levels

@100pah
Copy link
Member

100pah commented Mar 1, 2021

I'm not sure but will it be too complicated on design if we can have different colorBy on each node. Will it be much simpler and less confusing with the itemStyle.color if we only support colorBy on levels

@pissang At present, treemap colorMappingBy can be set on node/levels/series.
I'm not sure how many users user actually set colorMappingBy on node, but theoretically it works fine and not difficult in implementation when it represents "how to arrange the color of children".

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 levels.

@pissang
Copy link
Contributor

pissang commented Mar 1, 2021

but theoretically it works fine and not difficult in implementation when it represents "how to arrange the color of children".

I agree with this point. In this case, colorBy on each node will have no difference with series.colorBy or levels.colorBy. in definition and implementation

@Ovilia
Copy link
Contributor Author

Ovilia commented Jun 29, 2021

This PR does not change the behavior of treemap series but only the other series.
Treemap behavior on color mapping should be discussed and implemented in future PRs.

@Ovilia Ovilia marked this pull request as ready for review June 29, 2021 04:29
@Ovilia Ovilia requested a review from chfw June 29, 2021 04:35
src/util/types.ts Outdated Show resolved Hide resolved
@chfw
Copy link
Contributor

chfw commented Jun 30, 2021

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.

ecModel.eachSeries(function (seriesModel) {
if (!seriesModel.useColorPaletteOnData || ecModel.isSeriesFiltered(seriesModel)) {
ecModel.eachSeries((seriesModel: SeriesModel<SeriesOption & ColorByMixin>) => {
const colorBy = seriesModel.getColorBy();
Copy link
Contributor

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?

@pissang pissang added this to the 5.2 milestone Jul 19, 2021
@pissang pissang merged commit 1fb0d6f into master Jul 20, 2021
@echarts-bot
Copy link

echarts-bot bot commented Jul 20, 2021

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@pissang pissang deleted the feat-colorBy branch July 20, 2021 05:17
@Ovilia Ovilia mentioned this pull request Jul 20, 2021
3 tasks
Ovilia added a commit to apache/echarts-doc that referenced this pull request Aug 31, 2021
@Ovilia Ovilia removed the PR: awaiting doc Document changes is required for this PR. label Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants