-
Notifications
You must be signed in to change notification settings - Fork 16
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
Added simple LIFX example #2
Conversation
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.
Thanks you @GeerWille for your PR! There are some minor updates - but else it looks good to me!
device/LIFX/controller.js
Outdated
* Quick example for an LIFX light wrote by Geert Wille | ||
*/ | ||
|
||
'use strict'; |
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.
use strict should be at the first line of the file
device/LIFX/controller.js
Outdated
const BluePromise = require('bluebird'); | ||
|
||
let lifx = require('lifx-http-api'); | ||
let client = new lifx({ |
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.
lifx and client should be a const, they will never change
device/LIFX/controller.js
Outdated
*/ | ||
module.exports.onPulse = function (deviceId, name) { | ||
// Just randomly pulse the light | ||
client.pulse('id:' + name, { |
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.
is 'id:' + name
correct here?
can you make a helper function to get device selector, for example
function getLifxDeviceSelector(deviceId) {
return 'id:' + deviceId;
}
this would make the code much more readable
device/LIFX/controller.js
Outdated
* Discover all LIFX lights and return them in the proper format | ||
*/ | ||
module.exports.discoverLIFX = function discoverLIFX() { | ||
return client.listLights('all') |
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 you make a const for 'all' like:
const LIFX_ALL_DEVICES_SELECTOR = 'all';
so it's much more readable
About the |
Hi @GeertWille again, I just tested your implementation, some feedback:
|
…const where possible; Clean up
@neophob Changed everything as request above I think.
I also think there is a small mistake in the SDK. The function called with the addButtonHander has its parameters inverted. The first param is name and the second one deviceId. I think it should be switched around since most of the calls have deviceId first but If you think it shouldn't then the 2 examples should be changed. :) |
Hi @GeertWille BR |
I quickly wrote an extra example to show off how easy it is to attach new devices with the SDK. This one integrates a basic example of an LIFX light. I still have like the very first model that came out but I think it will work with any version. Enjoy!