Skip to content

Histogram2dcontour relayout fix #1520

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 3 commits into from
Mar 28, 2017
Merged

Conversation

etpinard
Copy link
Contributor

fixes #1517 and DRYs up the contours defaults logic for contour and histogram2dcontour traces 🌴

Thanks @rreusser for finding this bug 🔬 🪲

- which fixes the references issues on relayout
  when `autocontour: true`
@etpinard etpinard added status: reviewable bug something broken labels Mar 27, 2017
Copy link
Contributor

@rreusser rreusser left a comment

Choose a reason for hiding this comment

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

LGTM. Glad it was an easy fix!

var Lib = require('../../lib');
var attributes = require('./attributes');

module.exports = function handleContourDefaults(traceIn, traceOut, coerce) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Perhaps I'll need to merge/use this in carpet.

@@ -30,5 +31,6 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(autocontour) coerce('ncontours');
else coerce('contours.size');
Copy link
Contributor

@rreusser rreusser Mar 27, 2017

Choose a reason for hiding this comment

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

Can this line get the 🔪 ? It looks like handleContoursDefaults re-coerces this on the next line, but I could be mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh oops. Good call!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's subtle though. Sometimes these things do need re-coercing depending on the coupling between parameter defaults. But this one look like it's maybe redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in b163ad2

@alexcjohnson
Copy link
Collaborator

Looks great 💃

@etpinard etpinard merged commit a41b676 into master Mar 28, 2017
@etpinard etpinard deleted the histogram2dcontour-relayout-fix branch March 28, 2017 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

histogram2dcontour mock fails on interaction
3 participants