Add banlist action to channel context menus #1858

AlMcKinlay · 2017-12-19T11:43:09Z

No description provided.

AlMcKinlay · 2017-12-19T11:43:38Z

client/js/lounge.js

@@ -133,6 +133,14 @@ $(function() {
 					data: target.data("id"),
 				});
 			}
+			if (target.hasClass("channel") && utils.hasUserModeInChannel($(`#chan-${target.data("id")}`))) {


I'm not 100% on when to show this item.

On the one hand, on all the irc servers I use, any user can view the banlist, so should we show it for everyone? On the otherhand, I thought that only people with some sort of privileges in the channel would have a reason to view it, so we should probably only show it for them.

Open to suggestions.

Yeah I see no reason to limit this to user modes, just display it to everyone.

It might be nice to not show it for regular people as makes the functionality more accessible where people now might not be aware of it being possible unless they run a channel. It might result in some soapboxing in some channels if people are made more aware of the banlist.

Technically there is no reason not to show it indeed.

Yeah, my reasoning is 2-fold:

It makes the context list busier when there's probably no reason for you to be calling the banlist if you aren't at least voiced

Making it more accessible for users can lead to them unnecessarily calling it, and could lead to issues as explained by @creesch

If people think we should just show it for everything, that's fine. @astorije , what do you think with your UX hat on?

I can see both sides of the argument, but I am in favor of displaying it for normal users. I'll try to list my reasons here (don't see that as patronizing, I'm just too tired to gather thoughts properly and listing helps me compartimentalize :D).

Users can still send the /banlist command, so it's not like we're completely blocking the feature (though we could, but I don't think we should), we're just hiding it.

Adding a UI element for it helps with discoverability, which is nice.

The context menu becoming busier is a valid point, but at the moment we only have 1 real entry in there (2 if you count the entry that clicks on the target :) ) so I don't see that as an issue for now. We can for sure re-think that when it becomes a problem, and there are paradigms for that, such as grouping entries, or re-evaluate what we display in there.

You rightfully mention that this is primarily useful for "power users" or actually "voiced or op'ed users", which is a good reason why this shouldn't be in the main UI (i.e. constantly on the page, like Join a channel from the UI #1836 adds). But this is in a context menu, which is a good place where to put advanced features without burying them completely, I think.

Though it's clearly useful for voiced/op users, I can see how it's useful for regular users. Just like the question someone asked today "why would someone connect to the same network multiple times" (which is even less obvious IMO ahah), I'm sure there are plenty use cases for not being voiced on a channel you actively contribute/operate, or checking banlists on channels you don't operate, etc. In fact, I was working in a (private) team where no one was ever op or voiced to leave everyone on the same hierarchy level. Workflows 🤷‍♂️

Re: soapboxing (thanks, I learned a new word :D), I get what you're saying, but I don't think it's our business at least in this case. If we were a SaaS solution or something proprietary like that, I'd probably have a different point of view.

So yeah, I'd just display it :)

Alrighty, you've convinced me, I'll enable it for everyone.

Should I remove the changes I made to the op function, or should I put that in a different PR?

I have removed the changes to the op function and put them in #1864

AlMcKinlay · 2017-12-19T11:44:19Z

client/js/utils.js

@@ -48,6 +49,15 @@ function isOpInChannel(channel) {
 	return isOP;


As a side note, this only works for "op", doesn't work for "halfop" "owner", "admin" or anything else. So literally it tells you if a user is op, if the user is more privileged than op, it'll return false.

One thing that is difficult is that afaik only op and voice are documented in IRC RFC documents. Things like halfop, owner and admin are IRCd specific implementations.

Sure, but we currently have some support for them, so we should probably deal with them in this somehow.

AlMcKinlay · 2017-12-19T11:44:52Z

client/js/utils.js

@@ -48,6 +49,15 @@ function isOpInChannel(channel) {
 	return isOP;
 }

+// Given a channel element will determine if the lounge user has a user mode other than norml
+function hasUserModeInChannel(channel) {


We could probably find a nice way to combine these functions (allow the user to pass in a minimum privilege to check for.

Agreed, I probably made the isOp function a it narrow in scope as I had a very specific target in mind. Maybe make it possible to pass an array of possible modes to check for so the result can just be passedArray.contains(userMode)

That could be a nice way to do it.

Alright, I've updated it to this, defaulting to all of the roles (other than normal). Let me know what you think.

Yup looks good to me, much more versatile.

xPaw · 2017-12-19T11:48:28Z

client/js/lounge.js

+				output += templates.contextmenu_item({
+					class: "list",
+					action: "banlist",
+					text: "Show banlist",


Add a space in banlist.

astorije · 2017-12-20T01:02:42Z

client/js/lounge.js

+				output += templates.contextmenu_item({
+					class: "list",
+					action: "banlist",
+					text: "Show ban list",


Could you do "List banned users"? That is a bit more explanatory to me and fits better with the "List all channels" wording we did for #1802.

Makes sense, done

astorije · 2017-12-20T01:11:25Z

package.json

@@ -22,7 +22,7 @@
    "build": "webpack",
    "watch": "webpack --watch",
    "test": "npm-run-all --aggregate-output --parallel --continue-on-error test:* lint:*",
-    "test:mocha": "mocha --colors",
+    "test:mocha": "mocha --colors --require babel-polyfill --require babel-register",


Argh, running into that was bound to happen as we have more tests.
We can't do exactly that because that's babeling everything including server tests, and that means we could easily ship something incompatible with Node 4 or 6 on the server...

... But! I've been working on solving this and I'm almost there: #1726. There is one more dependency I rely on that needs releasing (the PR I opened to fix their repo is merged, they just need to release) but after that I'll be all set. That means that our client tests will run with the same config than our font-end builds, while the server tests remain unaffected.
I'm pretty excited about it because it brings a few good things, and opens the door to full-feature browser testing. Also I know it is not 100% ideal yet, but we can always improve from there.

(Also if they don't release before your PR is ready, we can find a workaround to not block your PR on it!)

Yeah, that's a valid reason.

But ... Surely the easiest solution is just having 2 different run configurations for client and server tests?

(this is now out of this pr and in #1864)

But ... Surely the easiest solution is just having 2 different run configurations for client and server tests?

That is pretty much what #1726 does, with test configuration running behind the (almost) same Webpack config than our client builds.

astorije

Nice!

AlMcKinlay commented Dec 19, 2017

View reviewed changes

xPaw reviewed Dec 19, 2017

View reviewed changes

AlMcKinlay force-pushed the yamanickill/context-menu-banlist branch 4 times, most recently from 2ea6dc1 to 7c33797 Compare December 19, 2017 16:43

astorije reviewed Dec 20, 2017

View reviewed changes

Add banlist context menu item

5084037

AlMcKinlay force-pushed the yamanickill/context-menu-banlist branch from 7c33797 to 5084037 Compare December 20, 2017 09:53

xPaw approved these changes Dec 20, 2017

View reviewed changes

xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Dec 20, 2017

xPaw added this to the 2.7.0 milestone Dec 20, 2017

astorije approved these changes Dec 21, 2017

View reviewed changes

astorije merged commit 31f1c2b into master Dec 21, 2017

astorije deleted the yamanickill/context-menu-banlist branch December 21, 2017 01:13

astorije changed the title ~~Add banlist context menu item if user isn't normal user~~ Add banlist action to the context menu Dec 21, 2017

astorije changed the title ~~Add banlist action to the context menu~~ Add banlist action to channels' context menu Dec 21, 2017

astorije changed the title ~~Add banlist action to channels' context menu~~ Add banlist action to channel context menus Dec 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add banlist action to channel context menus #1858

Add banlist action to channel context menus #1858

AlMcKinlay commented Dec 19, 2017

AlMcKinlay Dec 19, 2017

xPaw Dec 19, 2017

creesch Dec 19, 2017

AlMcKinlay Dec 19, 2017

astorije Dec 20, 2017

AlMcKinlay Dec 20, 2017

AlMcKinlay Dec 20, 2017

AlMcKinlay Dec 19, 2017

creesch Dec 19, 2017

AlMcKinlay Dec 19, 2017

AlMcKinlay Dec 19, 2017

creesch Dec 19, 2017

AlMcKinlay Dec 19, 2017

AlMcKinlay Dec 19, 2017

creesch Dec 19, 2017

xPaw Dec 19, 2017

AlMcKinlay Dec 19, 2017

astorije Dec 20, 2017

AlMcKinlay Dec 20, 2017

astorije Dec 20, 2017

AlMcKinlay Dec 20, 2017

AlMcKinlay Dec 20, 2017

astorije Dec 21, 2017

astorije left a comment

		@@ -48,6 +49,15 @@ function isOpInChannel(channel) {
		return isOP;

Add banlist action to channel context menus #1858

Add banlist action to channel context menus #1858

Conversation

AlMcKinlay commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije left a comment

Choose a reason for hiding this comment