swimlane / ngx-charts

:bar_chart: Declarative Charting Framework for Angular
https://swimlane.github.io/ngx-charts/
MIT License
4.29k stars 1.15k forks source link

labelFormatting method of pie series component is not flexible enough #1270

Open yuezhizizhang opened 4 years ago

yuezhizizhang commented 4 years ago

Describe the bug labelFormatting method is not flexible enough because it receives 'name' as parameter. It means I have to convert name to a label. However, if it accepts data as parameter, then I'm able to use another property like 'label' on the object to display.

To Reproduce Steps to reproduce the behavior: In src/pie-chart/pie-series.component.ts, on line 167: return this.labelFormatting(myArc.data.name);

Expected behavior Hope to pass data into it so that labelFormatting is able to use other properties on the object to work as label. return this.labelFormatting(myArc.data);

Screenshots If applicable, add screenshots to help explain your problem.

Demo Provide an online demo (stackblitz, codesandbox, or similar) where the issue can be reproduced

ngx-charts version Specify the version of ngx-charts where this bug is present

Additional context Add any other context about the problem here.

KingDarBoja commented 4 years ago

Hello @yuezhizizhang,

I do know how to accomplish such behaviour by letting the user pass a custom label formatting (as it does) but instead of using it with the default name parameter, would rather let the user handle it and if not label formatting, use the current one as fallback.

Still be aware that even a PR could take a while to get reviewed and merged.

Cheers!

PD: Nice blog website 🍰

yuezhizizhang commented 4 years ago

Hello @KingDarBoja

I'm struggling with labels with pie chart, including labels around the pie chart arc and the legend labels.

My data is like below:

this.series = 
    [{
         "name": 'Retired',
          "value": 20,
           "label": '20%'
     },
     {
        "name": 'Working',
        "value": 70,
        "label": '70%'
      },
      {
        "name": 'Children',
        "value": 10,
        "label": '10%'
      }];

I hope to display the percentage value around the pie chart arc. If labelFormatting only passes name as the argument, the only way for me to return the percentage value would be:

[labelFormatting]="pieChartLabel.bind(this, series)"

pieChartLabel(series: any[], name: string): string {
        const item = series.filter(data => data.name === name);
        if (item.length > 0) {
            return item[0].label;
        }
        return name;
}

It's not an efficient way, especially when the name field is localized. That's the reason I hope you could pass the data object itself to the labelFormatting.

Another question is about the legend labels. On line 139 of https://github.com/swimlane/ngx-charts/blob/master/src/pie-chart/pie-chart.component.ts:

getDomain(): any[] {
    return this.results.map(d => d.label);
}

domain is passed to and displayed on the legend component. By reading the codes, it looks like the legend should display the label property of the data. In my example above, it should be the percentage value.

However, I'm on @swimlane/ngx-charts@12.0.1. The legend always displays the name property. Do you know why?

yuezhizizhang commented 4 years ago

@KingDarBoja

Regarding the legend labels, by reading the codes, I find that in the https://github.com/swimlane/ngx-charts/blob/12.0.1/src/common/base-chart.component.ts, the results and data array are normalized, so that the label property is re-assigned a string value according to the name.

It's not so obvious because the results array is cloned. Hence, my results array are not altered but the the results array PieChartComponent uses is actually different.

KingDarBoja commented 4 years ago

Hello @yuezhizizhang

Thanks for the explanation, as you had seen, most components don't have clear types which makes debugging hard for collaborators like me. However, been submitting PR with changes that introduce such features so other get insight about method/variables behaviour across the charts.

I would like to help you but as you see, it requires a PR along with some minor changes to make it work. Unfortunately, even if I provide such PR, it would take a while to get reviewed and merged. Still gonna see if I got some free time to do it.

Cheers! 🍰