Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mtraynham
Copy link
Contributor

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
Copy link
Contributor Author

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.
    mtraynham@10ed200#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
Copy link
Contributor Author

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

@mtraynham
Copy link
Contributor Author

Doesn't fail locally either...

@gordonwoodhull gordonwoodhull added this to the v2.1 milestone Apr 21, 2015
@mtraynham
Copy link
Contributor Author

Passing again...

@mtraynham mtraynham force-pushed the heatmap_d3_svg_axis branch 2 times, most recently from 0f6cd09 to d050bb3 Compare December 2, 2016 19:41
@mtraynham mtraynham force-pushed the heatmap_d3_svg_axis branch 2 times, most recently from 8bbe8f1 to 6e88ce5 Compare January 15, 2017 16:51
@mtraynham mtraynham force-pushed the heatmap_d3_svg_axis branch from 6e88ce5 to 430e20e Compare January 19, 2017 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants