highcharts-for-python / highcharts-core

Python wrapper for the Highcharts Core JavaScript library
https://www.highcharts.com/integrations/python
Other
58 stars 13 forks source link

`distance` keyword for `data_labels` has no effect in pie charts #183

Closed JulienBacquart closed 4 months ago

JulienBacquart commented 4 months ago

Hi @hcpchris,

From the donut-charts.ipynb demos:

from highcharts_core.chart import Chart
from highcharts_core.options import HighchartsOptions

options_as_str = """{
    chart: {
        plotBackgroundColor: null,
        plotBorderWidth: 0,
        plotShadow: false
    },
    title: {
        text: 'Browser<br>shares<br>January<br>2022',
        align: 'center',
        verticalAlign: 'middle',
        y: 100
    },
    tooltip: {
        pointFormat: '{series.name}: <b>{point.percentage:.1f}%</b>'
    },
    accessibility: {
        point: {
            valueSuffix: '%'
        }
    },
    plotOptions: {
        pie: {
            dataLabels: {
                enabled: true,
                distance: -100,
                style: {
                    fontWeight: 'bold',
                    color: 'white'
                }
            },
            center: ['50%', '75%']
        }
    },
    series: [{
        type: 'pie',
        name: 'Browser share',
        innerSize: '50%',
        data: [
            ['Chrome', 73.86],
            ['Edge', 11.97],
            ['Firefox', 5.52],
            ['Safari', 2.98],
            ['Internet Explorer', 1.90],
            {
                name: 'Other',
                y: 3.77,
                dataLabels: {
                    enabled: false
                }
            }
        ]
    }]
}"""

options = HighchartsOptions.from_js_literal(options_as_str)
donut_chart = Chart.from_options(options)
donut_chart.display()

The distance keyword for the dataLabels has no effect, whatever the value.

Running:

donut_chart.to_json()

Return:

{
    "userOptions": {
        "accessibility": {"point": {"valueSuffix": "%"}},
        "chart": {
            "plotBackgroundColor": null,
            "plotBorderWidth": 0,
            "plotShadow": false,
        },
        "plotOptions": {
            "pie": {
                "center": ["50%", "75%"],
                "dataLabels": {
                    "enabled": true,
                    "style": {"fontWeight": "bold", "color": "white"},
                },
                "type": "pie",
            }
        },
        "series": [
            {
                "data": [
                    {"y": 73.86, "name": "Chrome"},
                    {"y": 11.97, "name": "Edge"},
                    {"y": 5.52, "name": "Firefox"},
                    {"y": 2.98, "name": "Safari"},
                    {"y": 1.9, "name": "Internet Explorer"},
                    {"y": 3.77, "dataLabels": {"enabled": false}, "name": "Other"},
                ],
                "name": "Browser share",
                "innerSize": "50%",
                "type": "pie",
            }
        ],
        "title": {
            "align": "center",
            "text": "Browser<br>shares<br>January<br>2022",
            "verticalAlign": "middle",
            "y": 100,
        },
        "tooltip": {"pointFormat": "{series.name}: <b>{point.percentage:.1f}%</b>"},
    }
}

Where the distance keyword is not present.

If I look at the data_labels class, I can't see no distance keyword. But the JS class seems to have it: https://api.highcharts.com/highcharts/series.pie.dataLabels.distance

Trying to run this JS demo for the pie chart

options_as_str = """{
    chart: {
        type: 'pie'
    },
    title: {
        text: 'Egg Yolk Composition'
    },
    tooltip: {
        valueSuffix: '%'
    },
    subtitle: {
        text:
        'Source:<a href="https://www.mdpi.com/2072-6643/11/3/684/htm" target="_default">MDPI</a>'
    },
    plotOptions: {
        series: {
            allowPointSelect: true,
            cursor: 'pointer',
            dataLabels: [{
                enabled: true,
                distance: 20
            }, {
                enabled: true,
                distance: -40,
                format: '{point.percentage:.1f}%',
                style: {
                    fontSize: '1.2em',
                    textOutline: 'none',
                    opacity: 0.7
                },
                filter: {
                    operator: '>',
                    property: 'percentage',
                    value: 10
                }
            }]
        }
    },
    series: [
        {
            name: 'Percentage',
            colorByPoint: true,
            data: [
                {
                    name: 'Water',
                    y: 55.02
                },
                {
                    name: 'Fat',
                    sliced: true,
                    selected: true,
                    y: 26.71
                },
                {
                    name: 'Carbohydrates',
                    y: 1.09
                },
                {
                    name: 'Protein',
                    y: 15.5
                },
                {
                    name: 'Ash',
                    y: 1.68
                }
            ]
        }
    ]
}"""

options = HighchartsOptions.from_js_literal(options_as_str)
donut_chart = Chart.from_options(options)
donut_chart.display()

Result in all labels overlapping because of the distance keyword missing: pie_chart_higharts_distance

hcpchris commented 4 months ago

Thanks, @JulienBacquart : This looks to be a bug. I'll look into it and resolve it in the next release.

JulienBacquart commented 4 months ago

Hi @hcpchris

Looking at the code code, I don't think it will work, because of this part:

    def distance(self, value):
        if not value:
            self._distance = None

According to the PEP8 :

Also, beware of writing if x when you really mean if x is not None – e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context!

I think you should be using:

if value is None:
    self._distance = None

Because if you use:

if not value:
    self._distance = None

You will return None for all cases where value is equal to [], {}, (), '', 0, False.

The problem is 0 is a correct value for distance. The default value is 30 : https://api.highcharts.com/highcharts/plotOptions.pie.dataLabels.distance So setting distance to 0 has an effect, as you can see here: https://jsfiddle.net/1s972jkr/

I think I run in the same problem in other places:

    'tooltip': {
        'headerFormat': '', 
        ...,

In my opinion, the empty string should be a valid value to override the default headerFormat, but this code does nothing. You have to do something like:

    'tooltip': {
        'headerFormat': 'null', 
        ...,
hcpchris commented 4 months ago

Thanks, @JulienBacquart : That's a good catch. Typically, if a false-y value is a valid value I employ the if value is None pattern, but in this case I failed to consider a distance value of 0. I'll patch that in a release of v.1.9.2 later today.

As for the tooltip.headerFormat, I'll take a closer look at that case and consider it in this release as well. There may be places where we're being overly broad in searching for False-y values, and other cases where we're not being broad enough.

JulienBacquart commented 4 months ago

Hi @hcpchris , The first example works fine now, but the second example doesn't.

It's probably because we have different hierarchies, as a parameter for a pie or a series:

plotOptions: {
        pie: {
            dataLabels: {
                distance: -100,
plotOptions: {
        series: {
            dataLabels: [{
                distance: 20,