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

Heatmap X axis tweaks #908

Closed
wants to merge 3 commits into from
Closed

Conversation

gazal-k
Copy link
Contributor

@gazal-k gazal-k commented Apr 8, 2015

The way the x axis was rendered for heatmap chart had one restriction for us, we couldn't rotate the tick labels like we could for coordinate grid charts, bubble chart etc.

Specifically, there was a single g.cols.axis element under which all the text elements were placed.
The x,y positioning was done separately for each text element.

This modification wraps each text element in a g.axis element where the x positioning is done using a translate transform. And the y positioning is done on the g.cols.axis element. This makes it easier to apply renderlets on the text elements for transformations such as rotation.

Didn't change the y axis rendering as we weren't sure whether it'll be useful. But if necessary, a similar modification can be made.

@mtraynham
Copy link
Contributor

Thanks for doing this. The more I look at it though, I'm under the impression we really should just use a d3.svg.axis, on both the x & y axes. Really not a fan of us trying to hack the functionality in, if something robust enough already supports it.

@gordonwoodhull
Copy link
Contributor

+1, I have no idea why we are reinventing axes here. If it's just for the click behavior, I'm pretty sure that could be added to d3 axes just fine.

@gazal-k
Copy link
Contributor Author

gazal-k commented Apr 15, 2015

That makes sense. Can the original authors of heat-map comment on why they reinvented the axes, just to be sure?

@mtraynham
Copy link
Contributor

I haven't seen @jrideout for quite some time... :(

Either way, I went ahead and created a second PR that satisfies this PR, uses d3.svg.axis and properly uses rangeBands() to calculate box widths/heights. Such a large library, it's good that these things are found.

@gazal-k
Copy link
Contributor Author

gazal-k commented Apr 16, 2015

thanks @mtraynham. +1, such a large library and well maintained. The test suite and lint checks are really well done. @mtraynham I noticed you haven't yet added/modified heatmap-spec. I could lend a hand if you don't mind.

@mtraynham
Copy link
Contributor

@gazal-k Hey that would be awesome! I'll be in the gitter chat if you want to talk about it.

@gazal-k
Copy link
Contributor Author

gazal-k commented Apr 16, 2015

Closing this PR.

@gazal-k gazal-k closed this Apr 16, 2015
@gazal-k gazal-k deleted the heatmap-x-axis branch April 16, 2015 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants