hunt down the rest of the bad selectAlls

[gordonwoodhull]

@mtraynham commented in #1032 (comment)

There seems to be a few places where selectAll is being used where select is likely more appropriate.

Bubble Chart:
https://github.com/dc-js/dc.js/blob/develop/src/bubble-chart.js#L117
https://github.com/dc-js/dc.js/blob/develop/src/bubble-chart.js#L133

Bubble Mixin:
https://github.com/dc-js/dc.js/blob/develop/src/bubble-mixin.js#L133

Geo:
https://github.com/dc-js/dc.js/blob/develop/src/geo-choropleth-chart.js#L158

Heatmap:
https://github.com/dc-js/dc.js/blob/develop/src/heatmap.js#L234
https://github.com/dc-js/dc.js/blob/develop/src/heatmap.js#L237
https://github.com/dc-js/dc.js/blob/develop/src/heatmap.js#L252
https://github.com/dc-js/dc.js/blob/develop/src/heatmap.js#L269

Line:
https://github.com/dc-js/dc.js/blob/develop/src/line-chart.js#L356

Row:
https://github.com/dc-js/dc.js/blob/develop/src/row-chart.js#L208

Verify that all the heatmap and bubble chart/mixin ones are fixed by #1032 / #1237. Track down the rest of them, create failing tests, and verify fixes for them.

[gordonwoodhull]

Checkbox the permalinks to these lines in today's develop branch:

bubble chart

• dc.js/src/bubble-chart.js

  Line 120 in 3168274

  .selectAll('circle.' + _chart.BUBBLE_CLASS)

• dc.js/src/bubble-chart.js

  Line 136 in 3168274

  .selectAll('circle.' + _chart.BUBBLE_CLASS)

bubble mixin

• dc.js/src/bubble-mixin.js

  Line 133 in 3168274

  var labels = bubbleGEnter.selectAll('text')

• (not listed above)

  dc.js/src/bubble-mixin.js

  Line 157 in 3168274

  g.selectAll('title').text(titleFunction);

[gordonwoodhull]

heatmap

fixed in #1032

• dc.js/src/heatmap.js

  Line 234 in 3168274

  boxes.selectAll('title').text(_chart.title());

• dc.js/src/heatmap.js

  Line 237 in 3168274

  dc.transition(boxes.selectAll('rect'), _chart.transitionDuration(), _chart.transitionDelay())

these are harmless but the selectAll of g.rows and g.cols above them is misleading - makes them look like a grouped selection when really it's just a simple axis-like g containing them. can be fixed without creating more tests. fixed in 0260b13

• dc.js/src/heatmap.js

  Line 252 in 3168274

  var gColsText = gCols.selectAll('text').data(cols.domain());

• dc.js/src/heatmap.js

  Line 269 in 3168274

  var gRowsText = gRows.selectAll('text').data(rows.domain());

[gordonwoodhull]

geo choropleth

This chart uses a non-standard data model, so these uses, although they look suspicious, actually work in the presence of data not updated in place. This is because the join is only between the map data and the elements, not between the crossfilter(ish) data and the elements. Instead the crossfilter data is read separately from its own array.

In fact, selectAll may be wanted in order not to rebind anything, because it should stay bound to the map.

I'm uncomfortable with this data model, and think that the crossfilter data should probably be bound with the map data before both are bound with the svg elements. This is the problem that causes it to be hard to e.g. add d3.tip support to the choropleth, because the crossfilter(ish) data is hidden. But I don't propose to fix that atm.

I also note that while #719 is a complete rewrite, and avoids some d3.select confusion through a lot of d3.select(x.parentNode).datum(), it still has the parallel data.

So this selectAll does not rebind data but it uses the parallel data[] instead:

• dc.js/src/geo-choropleth-chart.js

  Line 158 in 3168274

  regionG.selectAll('title').text(function (d) {

These ones look like they are selecting a single item but they are actually selecting all regions, and again the use of selectAll is probably intentional because the map is the only bound data:

• dc.js/src/geo-choropleth-chart.js

  Line 87 in 3168274

  .selectAll(layerSelector(layerIndex))

• dc.js/src/geo-choropleth-chart.js

  Line 170 in 3168274

  _chart.svg().selectAll('g.' + geoJson(layerIndex).name + ' path').attr('d', _geoPath);

  - probably should use layerSelector as well

I've verified with a bunch of new tests in aa27858 that the chart is not relying on data being modified in-place.

[gordonwoodhull]

line chart

overkill, ugly way to update a title but probably harmless. unfortunate to remove titles in order to update them, but the alternative is a very messy join on an array of one element and an explicit data carry-through. (the "add if not there" pattern is pretty gross if you're trying to pass data through from a previous selectAll.)

• dc.js/src/line-chart.js

  Line 397 in 3168274

  dot.selectAll('title').remove();

misleading as above

• dc.js/src/line-chart.js

  Line 54 in 3168274

  var layersList = chartBody.selectAll('g.stack-list');

these can be fixed without writing new tests.

[gordonwoodhull]

row chart

overkill as above

• dc.js/src/row-chart.js

  Line 210 in 3168274

  rows.selectAll('title').remove();

can be fixed without writing new tests.

line chart and row chart messiness fixed in f7e0a47

[gordonwoodhull]

in 2.0