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(polar) about ISSUE 11452 #11557

Merged
merged 3 commits into from
Nov 21, 2019
Merged

feat(polar) about ISSUE 11452 #11557

merged 3 commits into from
Nov 21, 2019

Conversation

foolzhang
Copy link
Contributor

No description provided.

@@ -424,8 +424,8 @@ function updateStyle(

el.useStyle(zrUtil.defaults(
{
fill: color,
opacity: opacity
fill: layout.startAngle === layout.endAngle ? 'transparent' : color,
Copy link
Contributor

Choose a reason for hiding this comment

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

fill and stroke can be set to 'none' if don't want to display it

fill: color,
opacity: opacity
fill: layout.startAngle === layout.endAngle ? 'transparent' : color,
opacity: layout.startAngle === layout.endAngle ? 0 : opacity
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is no need to set opacity to 0 if fill and stroke are both set to 'none'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fill : transparent , opacity: 0 , tooltip can be show if set, i just feel it better.
image
Is that right? @Ovilia

Copy link
Contributor

@pissang pissang Nov 5, 2019

Choose a reason for hiding this comment

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

I'm not sure if it's a good user experience if cursor becomes to poiner and tooltips shows on a blank area. Perhaps users will think is it a bug that there should be something under the mouse but instead it's blank ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK, i commit again

Copy link
Contributor

Choose a reason for hiding this comment

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

And silent property of the el should be set to true in case it displays again with emphasis style when user is hovering on the area.

Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

LGTM accept for what @pissang commented.

Ovilia
Ovilia previously approved these changes Nov 6, 2019
Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

I've tested test/polar-rounded.html and it looks good.

opacity: layout.startAngle === layout.endAngle ? 0 : opacity
stroke : layout.startAngle === layout.endAngle ? 'none' : stroke,
fill: layout.startAngle === layout.endAngle ? 'none' : color,
opacity: opacity
Copy link
Contributor

Choose a reason for hiding this comment

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

silent property of the el should be set to true in case it displays again with emphasis style when user is hovering on the area.

Sorry, I was wrong about this. silent property is not needed.

But the style of still needs to be eliminated. If I modify the test case and added emphasis style. 

series: [{
    type: 'bar',
    data: [0],
    coordinateSystem: 'polar',
    name: 'A',
    roundCap: true,
    color: 'rgba(200, 0, 0, 0.5)',
    itemStyle: {
        borderColor: 'red',
        borderWidth: 1
    },
    emphasis: {
        itemStyle: {
            borderColor: 'blue',
            color: 'red',
            opacity: 1
        }
    }
}]

The circle will appear when hovering on the legend.

image

I think we should also set fill and stroke to 'none' of hoverStyle in the code https:/apache/incubator-echarts/blob/master/src/chart/bar/BarView.js#L419

Copy link
Contributor Author

@foolzhang foolzhang Nov 12, 2019

Choose a reason for hiding this comment

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

sorry about not consider emphasis and last week I'm confused by what you say about "silent". lastweek i have tryed this

    if(layout.startAngle != layout.endAngle){
        graphic.setHoverStyle(el, hoverStyle);    
    }

it seems ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is any one here?

Copy link
Contributor

@pissang pissang Nov 18, 2019

Choose a reason for hiding this comment

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

Hi @foolzhang Sorry for missing this.

Just ignore the silent suggestion. I've made a mistake. It is for disabling the interaction ability of element.

About

 if(layout.startAngle != layout.endAngle){
        graphic.setHoverStyle(el, hoverStyle);    
}

It will cause the hoverStyle not change when the data is changed from a none zero value to zero.

We should always update all to avoid the status leaking from previous update.

if(layout.startAngle === layout.endAngle) {
  hoverStyle.fill = hoverStyle.stroke = 'none';
}
graphic.setHoverStyle(el, hoverStyle)

Copy link
Contributor Author

@foolzhang foolzhang Nov 21, 2019

Choose a reason for hiding this comment

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

We should always update all to avoid the status leaking from previous update.

reasonable

@pissang pissang merged commit 3b822f4 into apache:master Nov 21, 2019
@pissang pissang added this to the 4.6.0 milestone Nov 21, 2019
@pissang
Copy link
Contributor

pissang commented Nov 21, 2019

I found an issue after merged this pull request.

layout.startAngle === layout.endAngle

This is only available for the bar on polar. But there is no validation here and it brokes the bar on normal cartesian coordinate system.

I will make another pull request to fix this issue.

pissang added a commit that referenced this pull request Nov 25, 2019
fix(bar): fix bar won't display on cartesian caused by #11557
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.

3 participants