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

Cleaned up and simplified heatmaps #332

Merged
merged 2 commits into from
Aug 21, 2018
Merged

Cleaned up and simplified heatmaps #332

merged 2 commits into from
Aug 21, 2018

Conversation

jens-ox
Copy link
Contributor

@jens-ox jens-ox commented Aug 9, 2018

The heatmap demo overflows on the top (see the white half circle):
white circle

This is due to the way the y-positions of the heatmap elements are calculated. This can be simplified by using the array lengths to calculate the positions (imho the cleaner way).

This is what the updated Heatmap demo looks like:

heatmap updated


💥 Breaking Changes

  • Different vx-heatmap API

🚀 Enhancements

  • Cleaner vx-heatmap API, better Heatmap layout
  • Adds a separation attribute (margin between the two heatmaps) to the heatmap demo
  • Removes usage of data[0].bins.length as reference bins length, calculates maximum

🐛 Bug Fix

  • Fixes broken Heatmap demoo
  • Removes hardcoded left margin of 5

@jens-ox jens-ox changed the title cleaned up and simplified heatmaps Cleaned up and simplified heatmaps Aug 9, 2018
@hshoff
Copy link
Member

hshoff commented Aug 9, 2018

Hi @jens-ox thanks for the pull request!

I agree this would simplify the API only this would come at the cost of control. The heatmap API used to be simpler but was changed to address #302 in #307. Explanation: #302 (comment).

90% of the time users will likely scale on the index of the bin. The other 10% may need to do something else to support their data. Supporting that last 10% is a tradeoff. So I think for now we'll keep the API as is for now. Happy to continue the discussion.

I'd love to land your heatmap demo tile fix. I agree it looks much better with your improvements!

@jens-ox
Copy link
Contributor Author

jens-ox commented Aug 10, 2018

Hi @hshoff, thanks for your feedback! I don't see how this PR would pose a problem to scenarios like #302, as no bin function is used anymore. HeatmapRect would look like this:

<HeatmapRect
  data={data}
  xScale={xScale}
  yScale={yScale}
  colorScale={colorScale2}
  opacityScale={opacityScale}
  bins={d => d.values}
  binWidth={bWidth}
  binHeight={bWidth}
  gap={2}
/>

The only difference to the example is the additional bins attribute.

https://gist.github.com/jens-ox/83e85812a959b969a8c9feace61b64c8 is the code of the Heatmap demo using the same data structure as @JacquiManzi.

Copy link
Member

@hshoff hshoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I see now. LGTM

@hshoff hshoff added this to the v0.0.173 milestone Aug 21, 2018
@hshoff hshoff merged commit 309484f into airbnb:master Aug 21, 2018
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.

2 participants