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

fix: legend visual still with decal when user set 'legend.itemStyle.decal' to 'none' #16922

Merged
merged 9 commits into from
Apr 22, 2022

Conversation

jiawulin001
Copy link
Member

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Fix the problem that the legend patterns are still with decals when user sets 'legend.itemStyle.decal' to 'none'.

Fixed issues

Details

Before: What was the problem?

As is described in #16909 (comment), legend.itemStyle.decal is directly overwritten by series itemStyle without checking if it's set to 'none' by user.

before#16909

After: How is it fixed in this PR?

By adding the 'decal' into the ITEM_STYLE_KEY_MAP, Echarts now can correctly read the value of legend.itemstyle.decal set by users.
By checking whether user sets legend.itemstyle.decal to none, we decide whether decal should be overwritten by series itemStyle.

after#16909

Misc

  • 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

N.A.

Others

Merging options

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

Other information

@echarts-bot
Copy link

echarts-bot bot commented Apr 21, 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.

@@ -35,7 +35,8 @@ export const ITEM_STYLE_KEY_MAP = [
['lineDashOffset', 'borderDashOffset'],
['lineCap', 'borderCap'],
['lineJoin', 'borderJoin'],
['miterLimit', 'borderMiterLimit']
['miterLimit', 'borderMiterLimit'],
['decal']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This underlying method maps the itemStyle in echarts to the style object in zrender.

But decal in echarts is different with decal in zrender. In echarts it's an object that describe how the decal looks like. But in zrender it's a image that rendered as pattern on top of the element. So we didn't include decal here directly. Instead the decal property is created in

style.decal = createOrUpdatePatternFromDecal(decal, api);

Copy link
Member Author

@jiawulin001 jiawulin001 Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing it out. I agree I should use it to deal with interface decal. But the problem is that I cannot find other function than

const itemStyle = legendItemModel.getItemStyle();

to pick up attributes in legend.itemStyle.
Since getItemStyle() will only pick the attributes in the list of ITEM_STYLE_KEY_MAP, the only other way I think is to directly visit the decal attribute level by level, which I think is not elegant enough. And it could be risky if we call a structure like legendItemModel.ecModel.option.legend[0].itemStyle.decal, because if any structure in between does not exist or has changed, it could result in unstability. Is there any good one to pick the attribute decal without adding it to the list of ITEM_STYLE_KEY_MAP?

Copy link
Contributor

@pissang pissang Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Model#get or Model#getShallow is the best way to access the property in the option. getItemStyle is also using Model#getShallow at the bottom.

const val = model.getShallow(propName, ignoreParent);

These two method works like optional chaining operator. Some examples:

// Get the itemStyle object in series.
seriesModel.get('itemStyle')
// Get itemStyle.color value in series. Similar to seriesModel.option?.itemStyle?.color
seriesModel.get(['itemStyle', 'color'])

The difference between Model#get and Model#getShallow is Model#get will also consider cascading. If we didn't find itemStyle.color in the data level, it will keep finding in the series level and global option level.

The difference between Model#get and Model#getShallow is Model#get supports using array to get deep properties in object.

There is another method Model#getModel will wrap a new model after getting the value:

const itemStyleModel = seriesModel.getModel('itemStyle');
// itemStyleModel is now in the itemStyle scope
const color = itemStyleModel.get('color');

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your detailed explanation! My appreciate for your patience.
Now I use getShallow to get decal and use createOrUpdatePatternFromDecal to deal with it. But one last thing is to get api for createOrUpdatePatternFromDecal, which I found to be at legendItemModel.ecModel.scheduler.api. Is there a better way to access it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api will be given in the render method


You can pass it down to other methods if the method needs it.

BTW: I was wrong about Model#get and Model#getShallow. Just updated in the previous comment. get(path, false) in your commited code is the way to ignore option cascading.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just updated the code. I think I used getShallow in code and as I track the dataflow, it traces back to parent model if option is not found. So I think it works well in the code.

@pissang pissang added this to the 5.3.3 milestone Apr 21, 2022
@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 22, 2022
@pissang pissang merged commit 3961cef into apache:master Apr 22, 2022
@echarts-bot
Copy link

echarts-bot bot commented Apr 22, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] legend.itemStyle.decal is invalid
2 participants