Skip to content

Heatmap Padding #868

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

Merged
merged 4 commits into from
Aug 22, 2016
Merged

Heatmap Padding #868

merged 4 commits into from
Aug 22, 2016

Conversation

CalvinFernandez
Copy link

@CalvinFernandez CalvinFernandez commented Aug 19, 2016

Add xgap and ygap field to heat map that allows users to define space between bricks.

closes #868

Add padding field to heatmap that allows users to define space
between bricks.
@CalvinFernandez CalvinFernandez changed the title closes #858 Heatmap Padding Aug 19, 2016
@etpinard
Copy link
Contributor

@CalvinFernandez Thanks so much for this PR!

Looks like mocks hist2d_summed, 22 and 21 fail to render the heatmap.

Also, looks like jasmine test heatmap plot draws canvas with correct margins yields some cross-browser differences (we use FF on CI). You can try running that suite locally on FF with

npm run citest-jasmine -- tests/heatmap_test.js

If you have any questions about the two points above ⬆️ please let me know.

@etpinard
Copy link
Contributor

@CalvinFernandez tests are passing, this PR is looking great.

I just thought about one little detail that should be fixed before merging. Right now, xgap and ygap only have an effect for brick-like heatmaps and 2D histograms (i.e. with zsmooth set or infer as false). So we should make sure that xgap and ygap are coerced only when zsmooth allows them to have an effect. In other words, xgap and ygap are unsettable when zsmooth is set to 'best' or 'fast'. Unsettable attributes are caught by the Plotly.validate method allowing users to catch bad inputs.

For example:

var data = [{ y: [2,1,2], mode: 'markers', line: { shape: 'spline' } }];

Plotly.validate(data);
// => In data trace 0, container line did not get coerced

To do so, corce zsmooth before xgap and ygap in the heatmap and histogram2d defaults as such:

var zsmooth = coerce('zsmooth');

if(zsmooth === false) {
  coerce('xgap');
  coerce('ygap');
}

Thanks again for your efforts 🍻

@etpinard
Copy link
Contributor

Amazing PR. Thank you very much 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants