-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
- which fixes the references issues on relayout when `autocontour: true`
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh oops. Good call!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in b163ad2
Looks great 💃 |
fixes #1517 and DRYs up the
contours
defaults logic forcontour
andhistogram2dcontour
traces 🌴Thanks @rreusser for finding this bug 🔬 🪲