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

Pie chart adds filterValue parameter #8997

Merged
merged 11 commits into from
Jan 9, 2019
Merged

Conversation

cuijian-dexter
Copy link
Contributor

@cuijian-dexter cuijian-dexter commented Sep 3, 2018

The value of the fan page of the pie chart is less than a certain value. The label and labelLine of this fan page are not displayed.

options={
  title: {
    text: '饼图标题',
    left: 16,
    textStyle: {
      fontSize: 14
    }
  },
  tooltip: {
    trigger: 'item',
    axisPointer: {
      //坐标轴指示器,坐标轴触发有效type: 'shadow'//默认为直线,可选为:'line'|'shadow'
    },
    extraCssText: 'max-width: 300px; word-break:break-all; white-space: normal'
  },
  legend: {
    data: [
      
    ]
  },
  series: [
    {
      type: 'pie',
      label: {
        normal: {
          show: true,
          limitScale: 0.02//小于2%的,不显示
        },
        emphasis: {
          show: true,
          textStyle: {
            fontSize: '14',
            fontWeight: 'bold'
          }
        }
      },
      labelLine: {
        lineStyle: {
          color: '#D9D9D9'
        }
      },
      data: [
        
      ]
    }
  ]
};

…ue. The label and labelLine of this fan page are not displayed.
@cuijian-dexter cuijian-dexter changed the title The value of the fan page of the pie chart is less than a certain value. The label and labelLine of this fan page are not displayed. Pie chart adds limitScale parameter Sep 3, 2018
@100pah
Copy link
Member

100pah commented Sep 3, 2018

About the code:

(1) I think the option should not be on series.label, because both label and label line are controlled by the option. Probably it is better to be on series directly.

(2) Discussion:
I think the literal meaning of "limitScale" does not hint at what the feature does for users.
Any idea about the option name @Ovilia @pissang ?

(3) @cuijian-dexter the model should not be modified in the layout stage.
In fact, I think this feature should be implemented in the view rendering stage (PieView).

@cuijian-dexter
Copy link
Contributor Author

cuijian-dexter commented Sep 3, 2018

@100pah
Thank you for reminding me. I know what you mean.
I will repair it tomorrow and commit it again.

@Ovilia
Copy link
Contributor

Ovilia commented Sep 4, 2018

Is this limitScale the value for filtering labels? How about filterValue?

@cuijian-dexter cuijian-dexter changed the title Pie chart adds limitScale parameter Pie chart adds filterValue parameter Sep 5, 2018
cuijian and others added 4 commits September 5, 2018 11:35
@cuijian-dexter
Copy link
Contributor Author

@100pah
According to your suggestion, I modified my code.
1.Add the option to series
2.LimitScale changed to filterValue
3.Modify in PieView.js

Copy link
Member

@100pah 100pah left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply.
Thanks @cuijian-dexter .

But I think that filterValue probably still does not cover this feature.
em... it is a percent value or angle value, and used to hide label but display segment as usual.
But filter indicates that totally all value and corresponding graphic elements removed.

May be, although not neat, cropLabel? labelCropThreshold? labelHideThreshold?
@Ovilia

var itemModel = data.getItemModel(idx);
var valueDim = data.mapDimension('value');
var sum = data.getSum(valueDim);
var filterValue = seriesModel.getModel('filterValue');
Copy link
Member

Choose a reason for hiding this comment

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

// Should be
var xxxValue = seriesModel.get('xxxValue');

@Ovilia
Copy link
Contributor

Ovilia commented Sep 21, 2018

I'd better say labelDisplayThreshold. 😄

@cuijian-dexter
Copy link
Contributor Author

@Ovilia filterValue changed to labelDisplayThreshold.😄

@Ovilia
Copy link
Contributor

Ovilia commented Sep 21, 2018

Let @100pah have a final review.

@cuijian-dexter
Copy link
Contributor Author

@100pah hello,Can you make the final review?

src/chart/pie/PieView.js Outdated Show resolved Hide resolved
var sum = data.getSum(valueDim);
var filterValue = seriesModel.get('labelDisplayThreshold');
var filterIgnore = false;
if (sum * filterValue.option > itemModel.option.value) {
Copy link
Member

Choose a reason for hiding this comment

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

does seriesModel.get('labelDisplayThreshold') returns an object?

src/chart/pie/PieView.js Outdated Show resolved Hide resolved
src/chart/pie/PieView.js Outdated Show resolved Hide resolved
src/chart/pie/PieView.js Outdated Show resolved Hide resolved
@cuijian-dexter
Copy link
Contributor Author

cuijian-dexter commented Oct 17, 2018

@100pah check it again.Thanks

var option = {
            title: {
                text: '饼图标题',
                left: 16,
                textStyle: {
                    fontSize: 14
                }
            },
            tooltip: {
                trigger: 'item',
                axisPointer: {
                },
                extraCssText: 'max-width: 300px; word-break:break-all; white-space: normal'
            },
            legend: {
                data: [
                        
                ]
            },
            series: [
                {
                    type: 'pie',
                    labelDisplayThreshold:0.1,//范围为0-1,默认为0,全部显示。百分比小于0.1的,不显示
                    label: {
                        normal: {
                            show: true
                        },
                        emphasis: {
                            show: true,
                            textStyle: {
                                fontSize: '14',
                                fontWeight: 'bold'
                            }
                        }
                    },
                    labelLine: {
                        lineStyle: {
                            color: '#D9D9D9'
                        }
                    },
                    data: [{
                        name:'1212',
                        value:100
                    },
                        {
                            name:'1212',
                            value:49.9
                        },
                        {
                            name:'1212',
                            value:100
                        },
                        {
                            name:'1212',
                            value:100
                        },
                        {
                            name:'1212',
                            value:100
                        },
                        {
                            name:'1212',
                            value:50.1
                        },

                    ]
                }
            ]
        }

@Ovilia
Copy link
Contributor

Ovilia commented Oct 17, 2018

If you add a new configure option, please also update document both in English and Chinese.
Make a PR for https://github.com/ecomfe/echarts-doc and link it here.

Thanks!

@cuijian-dexter
Copy link
Contributor Author

@Ovilia ok,After the final review is completed,I will add again.
#8759 Has this been updated?

@Ovilia
Copy link
Contributor

Ovilia commented Oct 17, 2018

@cuijian-dexter Replied under #8759. :)

@Ovilia
Copy link
Contributor

Ovilia commented Oct 23, 2018

@100pah Please review this PR.

Copy link
Member

@100pah 100pah left a comment

Choose a reason for hiding this comment

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

Sorry for my late reply.

@@ -319,7 +327,8 @@ var PieView = ChartView.extend({
);

var selectedMode = seriesModel.get('selectedMode');

var valueDim = data.mapDimension('value');
data._sum = data.getSum(valueDim) || 0;
Copy link
Member

@100pah 100pah Oct 30, 2018

Choose a reason for hiding this comment

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

By convention, a property name prefixed by a single '_' represents a "private member" of a class. We should not add other that kind of properties (_xxx) outside the class definition dynamically. Otherwise, it probably confuses the code reader and might overwrite the inner private members with the same name.

If we need to save the sum here, it's OK to save it to this, because in this module we have the legal control of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,I can save sum to this.I change it

@cuijian-dexter
Copy link
Contributor Author

@100pah Already bind _sum to this

@100pah 100pah merged commit 5a3c7d0 into apache:master Jan 9, 2019
@100pah
Copy link
Member

100pah commented Jan 9, 2019

@cuijian-dexter Sorry for my late reply.
I have merged this PR. But something related is needed to be refactored (including this feature and some old feature). I will try to do it soon.

100pah added a commit that referenced this pull request Jan 9, 2019
…w the style of `minAngle` `startAngle`)

(2) move the logic to the "layout" phase, which is able to skip the process that avoid label overlap if do not display.
See more info in #8997
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants