search

dc-js / dc.js

Multi-Dimensional charting built to work natively with crossfilter rendered with d3.js
Apache License 2.0
7.42k stars 1.81k forks source link

hunt down the rest of the bad selectAlls #1239

Closed gordonwoodhull closed 7 years ago

gordonwoodhull commented 7 years ago

@mtraynham commented in https://github.com/dc-js/dc.js/pull/1032#issuecomment-155966554

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 commented 7 years ago

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

bubble chart

bubble mixin

gordonwoodhull commented 7 years ago

heatmap

fixed in #1032

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

gordonwoodhull commented 7 years ago

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:

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:

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

gordonwoodhull commented 7 years ago

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.)

misleading as above

these can be fixed without writing new tests.

gordonwoodhull commented 7 years ago

row chart

overkill as above

can be fixed without writing new tests.

line chart and row chart messiness fixed in f7e0a47

gordonwoodhull commented 7 years ago

in 2.0