dc.js icon indicating copy to clipboard operation
dc.js copied to clipboard

Using d3.svg.axis instead of custom axis implementation on heatmap

Open mtraynham opened this issue 9 years ago • 4 comments

This is probably the more desired change to the heatmap (instead of #908), which is to use d3.svg.axis instead of a one off implementation. I imagine our original heatmap implementation was borrowed from: http://bl.ocks.org/tjdecke/5558084 which also doesn't use d3.svg.axis.

The domain line and ticks have been set to opacity: 0 for consistency of look and feel.

Also as a one off, this properly uses the ordinal scales rangeBand() function to give the correct width/height of a heatmap box.

Working fiddle to show off the changes: http://jsfiddle.net/dybtxu3s/

Broke some tests though :( Will fix them first thing tomorrow.

mtraynham avatar Apr 16 '15 04:04 mtraynham

Tests fixed. Broke because:

  • Rename of the axis class from rows.axis -> axis.y; cols.axis -> axis.x; Reverted this.
  • Position is no longer checked by x & y attributes. We validate the transform attribute now.
  • Click events are triggered off the axis's g ticks, not the text.
  • And the last one I hate... dc.transition can return the selection or a transition. So the each call needs special handling as transitions have 2 parameters, the first being which state the transition is in. https://github.com/mtraynham/dc.js/commit/10ed200f557533bb9e123d8b63ad06236f85719e#diff-7485f5da2b41a10b58398ae4afbb4f61R223 We want the click handler applied after the transition.

Our tests always have a transition duration of 0, so the selection is always returned. When using the library as is, transitions are returned. Personally, I wouldn't mind if just a transition of duration 0 is returned, but I don't know the ramifications of that change.

Updated fiddle: http://jsfiddle.net/cwoj8uzm/

As a future request, it might be good to add padding to the rangeRoundBand as the boxes are not separated. Could be configurable. https://github.com/dc-js/dc.js/pull/919/files#diff-7485f5da2b41a10b58398ae4afbb4f61R195

mtraynham avatar Apr 16 '15 15:04 mtraynham

Hmm don't know why the stock tests failed when I updated a spec test...

mtraynham avatar Apr 17 '15 00:04 mtraynham

Doesn't fail locally either...

mtraynham avatar Apr 17 '15 13:04 mtraynham

Passing again...

mtraynham avatar Apr 22 '15 00:04 mtraynham