Skip to content

fix: issue#16749 colorAlpha check#16760

Closed
jiawulin001 wants to merge 2 commits intoapache:masterfrom
jiawulin001:issue#16749
Closed

fix: issue#16749 colorAlpha check#16760
jiawulin001 wants to merge 2 commits intoapache:masterfrom
jiawulin001:issue#16749

Conversation

@jiawulin001
Copy link
Copy Markdown
Member

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Fix the problem where itemStyle.colorAlpha does not apply to the view.

Fixed issues

Details

Before: What was the problem?

The attribute itemStyle.colorAlpha is not working to change the opacity.
A rectangle applying color = 'cyan' and colorAlpha = 0.3 has the same color as one applying only 'color = 'cyan'.
before

After: How is it fixed in this PR?

A simple check is added to Model.ts to see if colorAlpha comes together with color. However this is adding details into model, which I think is not good.
The result after fix is:
after

Code to reproduce
option = {
  series: [
    {
      type: 'treemap',
      label: {
        color: 'black',
      },
      data: [
        {
          name: 'nodeA',
          value: 10,
          color: ['red', 'pink'],
          children: [
            {
              name: 'nodeAa',
              value: 4,
              itemStyle: {
                color: 'cyan',
                colorAlpha: 0.3, // not work
              },
            },
            {
              name: 'nodeAb',
              value: 6,
              itemStyle: {
                color: 'cyan', // For Comparison
              },
            },
          ],
        },
        {
          name: 'nodeB',
          value: 20,
          children: [
            {
              name: 'nodeBa',
              value: 20,
              children: [
                {
                  name: 'nodeBa1',
                  value: 20,
                },
              ],
            },
          ],
        },
      ],
    },
  ],
};

Misc

  • The API has been changed (apache/echarts-doc#xxx).
  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

@echarts-bot
Copy link
Copy Markdown

echarts-bot bot commented Mar 28, 2022

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.

@pissang pissang added the discussion-required Further discussion is required before we decide if this issue should be fixed. label Mar 29, 2022
@pissang
Copy link
Copy Markdown
Contributor

pissang commented Mar 29, 2022

It will bring colorAlpha(and colorSaturation) to itemStyle of all series. I think we need further discussion if it's necessary.

First of all these two only do modification on the color property. It's a work that can be easily done by developers by changing the alpha value in rgba / 'hsl' string or using echarts.color.modifyAlpha and echarts.color.modifyHSL utility methods. I can't think of other benefits, or I may have missed something here, remind me if so. We tend to be very conservative in adding this kind of features.

If we do so, we need to do more work to keep the consistent. For example, areaStyle and lineStyle should also provide this ability. And what about borderColor, or even textStyle.color, shadowColor?

@jiawulin001
Copy link
Copy Markdown
Member Author

@pissang I agree, I think doing reduction is better than addition here. I would prefer removing colorAlpha from itemStyle because user can set opacity easily with rgba at color. Shall I submit another PR to try removing colorAlpha?

@pissang
Copy link
Copy Markdown
Contributor

pissang commented Mar 29, 2022

@jiawulin001 Sure, we can deprecate it progressively. For example, we can remove it in the doc at first. Then using warning method to tell developers not to use it anymore. Finally we can remove it in the major versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discussion-required Further discussion is required before we decide if this issue should be fixed. size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants