pie chart label() misdocumented, inconsistent

[namtab00]

Label function docs state

// label function has access to the standard d3 data binding and can get quite complicated
chart.label(function(d) { return d.data.key + "(" + Math.floor(d.data.value / all.value() * 100) + "%)"; });

... but argument (d) passed to the anonymous function is already d.data.

I discovered this while trying to calculate a piechart percentage label using d.startAngle and d.endAngle, which obviously are undefined.

Did I read the docs wrong or what gives?

[gordonwoodhull]

Thanks for pointing this out. This indeed annoying.

The pie chart is inconsistent with the bubble and row charts:

gordon$ grep label\(\) src/*
src/bubble-mixin.js:        return _chart.label()(d);
src/pie-chart.js:                return _chart.label()(d.data);
src/row-chart.js:                    return _chart.label()(d);

... and even the two examples in the documentation are inconsistent:

// default label function just return the key
chart.label(function(d) { return d.key; });
// label function has access to the standard d3 data binding and can get quite complicated
chart.label(function(d) { return d.data.key + "(" + Math.floor(d.data.value / all.value() * 100) + "%)"; });

To fix this, we'd probably have to break all pie charts that use .label(). So the best I can do for 2.0 is to add a warning to the documentation. In 2.1 we can break the pie chart labels, perhaps. Maybe with a console warning.

[gordonwoodhull]

Tempted to make these consistent throughout, even if it breaks code, for 2.0.

[gordonwoodhull]

We will break the interface to make these consistent for 2.1

[MarkHerhold]

@gordonwoodhull Why not break the interface for 2.0? 2.0 is a major semver release, so it actually makes more sense to me if it were to go goes into 2.0 than 2.1 (minor).

Besides, I would love to have this fix in for my own purposes. 😄

[gordonwoodhull]

We're already in beta on 2.0, and have been for a year or more, so I want to maintain the consistent interface there. Breaking everyone's apps as they upgrade betas isn't very nice.

As for semantic versioning, I'm doing something a little unorthodox because dc.js has so much surface area (and was developed very quickly): the minor versions introduce breaking interface changes. Otherwise the major version would go through the roof, because many improvements break the interface.

[MarkHerhold]

@gordonwoodhull All very good points to which I can't argue. :)

I looked briefly at doing a monkey patch, but the positionLabels function isn't on the dc.pieChart prototype so I don't think that is an option either. Was keeping the functions off of the prototype object a design decision?

[gordonwoodhull]

Well, just in the sense that we try not to expose all the internals; normally what we do is expose functions as we see a need for customization (but try to have decent APIs rather than just lots of hooks, where possible).

I guess if you want a workaround for 2.0, what we'd need to do is expose labelPosition as chart._labelPosition, and expose positionLabels as... hmm, that's not really what it does, maybe chart._applyLabelAttrs.

Alternately, you could probably use a pretransition hook to change the labels before they're drawn to the screen.

[gordonwoodhull]

Okay, I tried it out and that didn't quite work, so we need a patch for either approach.

The application of the label text during the transition is probably wrong, so I'm pulling that out to its own patchable function chart._applyLabelText and applying it immediately.

Now you can have your choice of:

    chart._applyLabelText = function(labels) {
        labels.text(function(d) {
            console.log(d);
            return d.data.key + ' ' + dc.utils.printSingleValue((d.endAngle - d.startAngle) / (2*Math.PI) * 100) + '%';
        });
    };

or I somewhat prefer (and I'm adding this to the pie example):

    chart.on('pretransition', function(chart) {
        chart.selectAll('text.pie-slice').text(function(d) {
            return d.data.key + ' ' + dc.utils.printSingleValue((d.endAngle - d.startAngle) / (2*Math.PI) * 100) + '%';
        })
    });

[gordonwoodhull]

(These changes will be available in 2.0 beta 26 this afternoon.)

[MarkHerhold]

YES! Thank you @gordonwoodhull !
I will give these a shot once beta 26 is cut. 👍