-
Notifications
You must be signed in to change notification settings - Fork 113
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
Add the option to make the application run on startup for all users #74
base: master
Are you sure you want to change the base?
Conversation
anaszgh
commented
Dec 10, 2017
- Target Platform: Windows
- Unable to make the application run on startup for all users
- Could it break any existing functionality for users? Nope
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.
Hey, thanks for this! I've just reviewed it. You should update the README too by the way.
I can't test it right now but I will later.
@@ -52,8 +58,11 @@ module.exports = | |||
|
|||
|
|||
# appName - {String} | |||
# onlyMe - (Optional) {Boolean} |
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.
A comment for mac
is missing
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.
Please, use a name that is more representative of what's the purpose of the parameter. We usually talk about a user-wide or a system-wide installation (something like "systemWideInstall").
@@ -12,13 +12,14 @@ module.exports = class AutoLaunch | |||
# to add Login Item | |||
# :name - {String} | |||
# :path - (Optional) {String} |
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.
You should add a new comment for the new option
throw new Error 'You must specify a name' unless name? | ||
|
||
@opts = | ||
appName: name | ||
isHiddenOnLaunch: if isHidden? then isHidden else false | ||
mac: mac ? {} | ||
mac: mac ? {}, | ||
onlyMe:onlyMe?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.
Can you add some spaces here please?
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.
Same for a few other areas actually
As stated under #47, maybe we should regroup additional parameters under an array of options so the constructor's parameter list doesn't get longer than needed. |
Is it possible to offer the same functionality for macOS and Linux? Yes/No, document the state please. I think I saw something under Chromium/Chrome that could allow us to also do it for Linux. |