Skip to content
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

Merged
merged 1 commit into from Mar 2, 2016
Merged

Allow locking network configuration #82

merged 1 commit into from Mar 2, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Feb 21, 2016

Fixes #23, #46, and removes hardcoded default to freenode server in code (#13 (comment)).

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Feb 21, 2016
// @type boolean
// @default false
//
lockNetwork: false,
Copy link
Member

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.

Copy link
Member Author

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.

@astorije astorije assigned AlMcKinlay and unassigned AlMcKinlay Feb 25, 2016
@astorije
Copy link
Member

@xPaw, now that I pay attention, what is the difference between this and displayNetwork in practice?

I mean, not displaying the network does connect you to the network already set in the config file, correct?

@xPaw
Copy link
Member Author

xPaw commented Feb 25, 2016

displayNetwork does what it says. It just hides the network using display:none, it doesn't actually prevent users from joining another network. And it's better to keep it separate, as you might want to lock the network, but still display it (like our demo instance does).

@astorije
Copy link
Member

Then I'm wondering what good brings displayNetwork on its own... Should we remove it altogether? Or at least enable lockNetwork by default when enabling displayNetwork?

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.

@xPaw
Copy link
Member Author

xPaw commented Feb 25, 2016

Yeah I'm not sure about it, but it should be handled in a separate PR.

@maxpoulin64
Copy link
Member

I agree with keeping displayNetwork seperate. One use I find is when The Lounge is used as a web gateway to a network, it could be useful to lock it to that network, but still keep the settings visible to advertise where it connects to. This could be handled otherwise (like putting it on the network's website), but I like the transparency of what it's doing.

EDIT: Removed my 👍 after more testing. Some codepaths are not working.

@astorije
Copy link
Member

astorije commented Mar 1, 2016

@xPaw, @maxpoulin64, where are we on this one? Do you think it could go along with the next minor release later this week? :-)

@maxpoulin64
Copy link
Member

@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' : ''%>>
Copy link
Member

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 ''%>>.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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)

@astorije
Copy link
Member

astorije commented Mar 2, 2016

Alright, 👍 and merging. Comments to minor to justify a hold off.

astorije added a commit that referenced this pull request Mar 2, 2016
Allow locking network configuration
@astorije astorije merged commit ec37b66 into thelounge:master Mar 2, 2016
@xPaw xPaw deleted the lock-server branch March 7, 2016 17:17
@astorije astorije added this to the 1.3.0 milestone Apr 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants