-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Give Thermometer enable/disable methods #1346
Give Thermometer enable/disable methods #1346
Conversation
Updates board snapshot and serialize tests to account for the new thermometer `freq` property.
This is what I try to do, but not what always ends up happening, for a variety of reasons.
Such is the result of a relaxed contribution policy, but I'm ok with the incurred technical debt (for now).
Don't worry about that for now :)
This should be fine for now. |
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.
Brilliant!
Can you update the following:
- Add
enabled
to: https://github.com/rwaldron/johnny-five/wiki/Thermometer#parameters - Add
freq
to: https://github.com/rwaldron/johnny-five/wiki/Thermometer#shape - Add
enable()
anddisable()
to: https://github.com/rwaldron/johnny-five/wiki/Thermometer#api
freq: opts.freq || 25, | ||
previousFreq: opts.freq || 25, | ||
}; | ||
priv.set(this, state); |
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.
👍
state.intervalId = setInterval(eventProcessing, newFreq); | ||
} | ||
} | ||
}, |
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.
👍
if (state.enabled) { | ||
this.freq = state.freq; | ||
} | ||
} |
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.
👍
}.bind(this), freq); | ||
} | ||
return this; | ||
}; |
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.
👍
} | ||
|
||
return this; | ||
}; |
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.
👍
}, | ||
enable: testEnable, | ||
disable: testDisable, | ||
constructDisabled: testConstructDisabled, |
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: Documented |
Addresses #1297 by giving the
Thermometer
componentenable()
anddisable()
methods.I've added tests but I'm a little confused by the structure of the Thermometer unit tests - this feature lives on the prototype and should be the same for all controllers, but it looks like it has to be tested per-controller? But similar tests aren't applied to all controllers either. Let me know if I should make sure these tests get added to every controller, or if we can run them just once on the default controller, etc.
Thanks!