-
Notifications
You must be signed in to change notification settings - Fork 700
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
Allow locking network configuration #82
Conversation
// @type boolean | ||
// @default false | ||
// | ||
lockNetwork: false, |
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.
Instead of locking to the default, could we have an array of hostnames that we can lock to. Would allow people that want to allow multiple irc servers, but not just any random one.
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.
That would require some restructuring, plus have a dropdown on the client show allowed servers. I'm not keen on doing that myself.
@xPaw, now that I pay attention, what is the difference between this and I mean, not displaying the network does connect you to the network already set in the config file, correct? |
|
Then I'm wondering what good brings I would simply remove it after looking where it comes from and why. Probably a different PR though but discussion is interesting if we want to lock when not displaying it. |
Yeah I'm not sure about it, but it should be handled in a separate PR. |
I agree with keeping EDIT: Removed my 👍 after more testing. Some codepaths are not working. |
@xPaw, @maxpoulin64, where are we on this one? Do you think it could go along with the next minor release later this week? :-) |
@astorije Yep, looks good to me now that my questions have been answered. 👍 |
@@ -120,7 +120,7 @@ <h1 class="title">Connect</h1> | |||
<div class="col-sm-3"></div> | |||
<div class="col-sm-9"> | |||
<label class="tls"> | |||
<input type="checkbox" name="tls" <%= defaults.tls ? 'checked="checked"' : '' %>> | |||
<input type="checkbox" name="tls" <%= defaults.tls ? 'checked="checked"' : '' %> <%= typeof(lockNetwork) !== "undefined" && lockNetwork ? 'disabled' : ''%>> |
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.
[Very minor comments] For these 3 changes, could you choose either single or double quotes, rather than a mix of both? Also, you might want to do '' %> >
or '' %>>
instead of ''%>>
.
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.
This seems reasonable to me, to avoid the annoying escaping of the double-quotes for the HTML. This is consistent with what's already there. Double-quotes for Javascript expressions, single quotes for the HTML output.
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.
I thought about that at first, but actually there is no need to escape here, unless I am being mistaken at that time of the day: <%= typeof(lockNetwork) !== "undefined" && lockNetwork ? "disabled" : "" %>
seems to be fine to me.
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.
(And actually, for checkboxes, no need to do 'checked="checked"'
, "checked"
would be enough here)
Alright, 👍 and merging. Comments to minor to justify a hold off. |
Allow locking network configuration
Fixes #23, #46, and removes hardcoded default to freenode server in code (#13 (comment)).