Composite charts don't handle rangeBandPadding

[jcamins]

If you create a composite chart with (for example) an ordinal bar chart, rangeBandPadding is not set for the sub charts, but still has an effect, so you have to set rangeBandPadding on all the subcharts using .barPadding, and also set the rangeBandPadding using ._rangeBandPadding on the composite chart.

I would be happy to fix this and submit a PR but I can't figure out where subcharts inherit settings from the parent. Any pointers?

[gordonwoodhull]

Thanks. Seems like there should be a more comprehensive way to do this, but instead there is compositeChart.compose:

https://github.com/dc-js/dc.js/blob/master/src/composite-chart.js#L245

and compositeChart.generateG:

https://github.com/dc-js/dc.js/blob/master/src/composite-chart.js#L61

[gordonwoodhull]

Probably generateG is the right place for this.

[jcamins]

It turns out this is actually a little bit more complicated than I thought, because the default gap on bar subcharts breaks rendering on composite charts:

var data = [{"key":0,"value":1},{"key":10,"value":25},{"key":20,"value":71},{"key":30,"value":99},{"key":40,"value":13},{"key":50,"value":4},{"key":60,"value":120},{"key":70,"value":110},{"key":80,"value":37},{"key":90,"value":0}];

var _ndx = crossfilter(data);
var _dimension = _ndx.dimension(dc.pluck('key'));
var _group = _dimension.group().reduce(function (p, v) { return v.value; }, function (p, v) { return v.value; }, function () { return 0; });
var _chart = dc.compositeChart('#indicators')
    .height(300)
    .width(1020)
    .xUnits(dc.units.ordinal)
    .x(d3.scale.ordinal())
    .elasticY(true)
    .dimension(_dimension)
    ._rangeBandPadding(0.5)
    .group(_group);

var subchart = dc.barChart(_chart)
    .gap(undefined);

_chart.compose([subchart]);
dc.renderAll();

Without that .gap(undefined) (or a .barPadding(0.5) which undefines the gap as a side effect), the bars will overlap and not look good at all. If I remove all padding-related calls in that code (i.e. ._rangeBandPadding, .gap, .barPadding), the bar chart still won't render correctly as a subchart, so to me the obvious solution would be to check if the parent is a dc chart, and if it is, don't set _gap to the default at https://github.com/dc-js/dc.js/blob/master/src/bar-chart.js#L44

What do you think of that approach?

[jcamins]

I've been thinking about this more. Why does gap take precedence over barPadding for determining the width? What about changing it so that barPadding takes precedence, if it's been set, but gap has a default of 2 if barPadding hasn't been set?

[gordonwoodhull]

That sounds like it would preserve the current behavior, but in a simpler more direct way. If so, the change would be welcome. Please try it out and see if the current tests still pass. That won't quite guarantee that it's a non-breaking change, but it would give some assurance.

[jcamins]

Progress report: I have a branch that doesn't quite work at https://github.com/jcamins/dc.js/tree/issue678 . Two unit tests are still failing:
X dc.barChart with changing number of ordinal bars and elasticX should not overlap bars
Expected 126 to be less than 126.
Expected 210 to be less than 210.
Expected 294 to be less than 294.
X dc.barChart with changing number of ordinal bars and elasticX with bars added should not overlap bars
Expected 90 to be less than 90.
Expected 150 to be less than 150.
Expected 210 to be less than 210.
Expected 270 to be less than 270.
Expected 330 to be less than 330.

There's still a logic bug somewhere in the default gap code.

[gordonwoodhull]

@jcamins, this code is really a mess, isn't it? There are two competing ways of determining the gaps. My understanding is that this is partly because we can't use rangeBands for charts with a quantitative scale, and some of the fiddly logic is due to trying to keep backward compatibility.

However, your fix should clean it up a bit, and if you look at the failing tests above, they look like they're just an off-by-one bug - "expected 90 to be less than 90" etc. If you have time, would you like to to give it another try and submit a PR?