made channel names in chat clickable; lets users join channels #385

AmShaegar13 · 2015-03-28T18:49:54Z

Implements #361.

astorije · 2015-09-26T01:26:36Z

client/css/style.css

@@ -573,6 +573,7 @@ button {
 }
 #chat .wrap,
 #chat .text a {
+	cursor: pointer;


Is it really necessary to add a cursor pointer to a links as opposed to default behavior?

Well, what means necessary. If you want the links to appear and behave the same as normal html links, then yes. It is necessary. If you are satified with the default behavior, which means the cursor changes to text, then it is not necessary.

I like the first option.

Sorry, I didn't see the #chat .wrap on the first line. However, I'm confused, all links are wrapped into a links (what you called normal HTML links). Do you have an example where we need this?

MIght be that one of us is missing something. My point is, when I create a link like so: <a data-chan="#channel"> it will not have the pointer cursor due to the missing href. At least firefox behaves like that. To solve that I added cursor: pointer;.

Ah, very good point! I had forgotten about this oddity. Well, the HTML5 spec does say that the a element, absent an href, is not an hyperlink but a placeholder.

This Stack Overflow is very well built and even links to WAI-ARIA!

My conclusion on this (also after reading this) is that we should not go for an a link. We should either go for a button or a span with the right ARIA role and focus behavior. At the moment links on the left menu are using a span, links on the right menu are using a button. Please go for a button and if you realize it's hell to style (not sure how it behaves with long statements that go to a second line) then switch back to a span:

<button role="button" tabindex="0">...</button>

This is definitely the right way to go for in-app "links" and I'll make sure we go for the right route in subsequent choices. And we keep a links for actual hyperlinks.

Does that make sense to you? Let me know if you have any questions, and sorry for being slow at first :-)

Ah, sounds good to me. Let me change that tomorrow.

Awesome! Thanks for being so responsive, I really appreciate it! Talk to you soon.

astorije · 2015-09-26T01:28:33Z

I'll try to give a review to this PR. Will look for @erming or @JocelynDelalande's eyes as well.

Xe · 2015-09-26T17:19:30Z

client/js/shout.js

+				var html = $(this).html();
+				$(this)
+					.addClass("processed")
+					.html(html.replace(/(\W)(#[\w\d-.]{0,200})/ig,


This will match channels longer than the IRC daemon likely supports. Does this need to be 200?

I searched for the specification when creating the regular expression and came up with this:

Channels names are strings (beginning with a '&' or '#' character) of
length up to 200 characters.

https://tools.ietf.org/html/rfc1459#section-1.3

Thanks for linking to an RFC 👏
Actually, it's so useful to have it there, could you make it a comment shaped like this please:

// Channels names are strings (beginning with a '&' or '#' character) // of length up to 200 characters. // See https://tools.ietf.org/html/rfc1459#section-1.3 .html(...

Three other comments:

I don't see support for &channel, as only # is present in your regex. Could you add support for this please?

Even though the RFC doesn't explicitly say it, I don't think # is a legal channel. So maybe minimal length should be 1 instead of 0

The RFC says, however, that channel names are up to 200 characters, #/& included :-) So it should be 199 and not 200 :-)

I couldn't find a network where &channel is working. Are you sure that this is supported by shout's backend?

Actually, I do think so. Test it on the shout demo network for example. /join # works just fine.

I agree.

I couldn't find a network where &channel is working. Are you sure that this is supported by shout's backend?

Typing /join <whatever> in the input is simply a forward to Slate with <whatever> as an argument, which in turn merely sends JOIN <whatever> to the network, so I don't think there any massaging on that. Actually, it might be nice to prevent trying to join a network that does not respect the RFC, either on Shout or on Slate, but that's for another PR ;-)

Additionally, I asked @deiu as I know he is using Shout on a &-enabled server and he confirmed it works fine.

Actually, I do think so. Test it on the shout demo network for example. /join # works just fine.

You are correct (and it highlighted #483 ^^), so the right interval is {0,199} indeed!

richrd · 2015-09-27T19:36:31Z

Sorry for not contributing anything, but I just want to say that I support this feature. I think it's a good addition.

AmShaegar13 · 2015-09-27T19:48:24Z

Thanks for your feedback. Added it to the code. Also I made a litte change to the regex to comply with the rfc completely. Originally, it only allowed a-zA-Z0-9. It's now accepting every character except those mentioned in the RFC: space, comma, \x07

astorije · 2015-09-27T19:48:25Z

client/js/shout.js

+					// Channels names are strings (beginning with a '&' or '#' character)
+					// of length up to 200 characters.
+					// See https://tools.ietf.org/html/rfc1459#section-1.3
+					.html(html.replace(/(\W)([#&][^\x07\x2C\s]{0,199})/ig,


Why did you replace [\w\d-.] with [^\x07\x2C\s]? What does the latter one mean? Seems more cryptic (if you consider regexes aren't cryptic in the first place...)

@AmShaegar13 answered here.

RFC states:

the only restriction on a
channel name is that it may not contain any spaces (' '), a control G
(^G or ASCII 7), or a comma (',' which is used as a list item
separator by the protocol)

astorije · 2015-09-27T20:08:44Z

Nice @AmShaegar13!
I am testing this locally (since the code looks nice :) ), and here are my discoveries:

Style between normal text and channel names acting as buttons are the same. I won't be blocking this PR for that as I'm OK with it, but an alternative I would display them same as links. Let's see what the second reviewer says.
It works fine when the channel isn't already in the opened channel from the left list, because it makes you join and open the channel in foreground. If you already have joined this channel, nothing happens. This seems like a bug and what I suggest to solve it is to open the already joined channel. Could you do this? (maybe emit a click?)
Since this is now a button, long lines won't cut the channel name but put it on another line:

Second line shows the exact same text without the #. I think this should be consistent, the channel name should not be on its own line. Could you try to see if you can have button to do that? If not because of the way button is, then feel free to go for a span instead ;-)
And by the way, opening the channel above...

Ouch, can you fix what's going on with that trailing </i> please?

AmShaegar13 · 2015-09-27T21:06:55Z

I discovered another downside of the button: You cannot select it as text. We should really use span... This should fix long channel names.

<i class="wrap"></i> is added around long messages. Didn't see that... The regex matches the closing now... Otherwise it would always be a space or new line.

This took me to another problem: HTML entities are escaped. So instead of joining #channel<s> you would join #channel<s>... Looks like the HTML context is not good for this feature... We may have to rethink where this code belongs 😕

astorije · 2015-09-28T00:51:50Z

I discovered another downside of the button: You cannot select it as text. We should really use span... This should fix long channel names.

Yes, go for it :-)

The regex matches the closing now...

You probably didn't push this, did you?

Looks like the HTML context is not good for this feature... We may have to rethink where this code belongs

I don't think we should: this definitely belongs to the client, not the server! Plus, in the msg incoming from the socket is also fine too.
However, I do realize some problems indeed. That processed thing is odd and we shouldn't need it: we are actually processing one message, so let's add the span button here only, not trying to loop through all non-processed lines (which should be just this one!).
There are solutions to the HTML tag issue, but we must be careful as it could add security concerns with XSS if not correctly handled. I'll sleep on it and let you fix the rest in the meantime. Thanks!

AmShaegar13 · 2015-10-02T19:09:52Z

Here we go. First of all, I replaced button by span. I had to add the rule for the pointer cursor again now. Secondly, I cleaned up the 'processed' thing. I have no ide why I did this in the first place... I made the new message a jQuery object and used this for further processing.

I fixed the issue by working directly on the element, if there is one.

Also, I broke travis-ci, although I'm not sure if it's really my fault. The output says something about a timeout. I can still fetch link information on my development instance.

Still missing the open-already-joined-channels thing.

Please let me know what you think!

JocelynDelalande · 2015-10-04T18:44:23Z

Also, I broke travis-ci, although I'm not sure if it's really my fault. The output says something about a timeout. I can still fetch link information on my development instance.

You didn't :)

Actually, it was broken already, now it's fixed, see #493 please update your PR rebasing on master, tests should be passing then.

astorije · 2015-10-04T18:49:21Z

Hi @AmShaegar13, this looks very good, exciting!
As you can see, I left a few inline comments. They are basically only style-related at this stage, meaning we are very close :-)

Trying the changes in a local clone, here is what I notice though:

There is no style applied when :hovering the channel names. Can you add the opacity thingy that is somewhat consistent over the UI?
I tried to test the multiline channel names with #very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-long-channel but on Freenode it doesn't work. Shorter names that expand on more than one line do work fine, so maybe it's a limitation from the Freenode server?!

As @JocelynDelalande just said, don't worry about the tests, just rebase and you'll be fine :-)

JocelynDelalande · 2015-10-04T20:23:33Z

I tried to test the multiline channel names with #very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-long-channel but on Freenode it doesn't work. Shorter names that expand on more than one line do work fine, so maybe it's a limitation from the Freenode server?!

if more than 50 chars it's not an irc chan name.

astorije · 2015-10-05T01:46:25Z

Waouh! @AmShaegar13 and I got tricked by [an older RFC]! Thanks @JocelynDelalande!

@AmShaegar13, could you update your regex to accept the 4 prefixes followed by 49 characters?

Xe · 2015-10-05T02:18:17Z

@JocelynDelalande that RFC is near universally rejected by IRC daemon developers.

astorije · 2015-10-05T02:26:02Z

@JocelynDelalande that RFC is near universally rejected by IRC daemon developers.

This is one of these very strong statements that could really use a reference.

Fact is, it seems to be true on Freenode, at least... (or shout itself refuses to join, which is a different but still relevant problem)

AmShaegar13 · 2015-10-10T11:03:16Z

Okay, implemended your feedback but there are still two little issues I just found out.

~~The <i> that is wrapped around long lines has no spaces in front of the text. Thus, channels at the beginning of a line do not match anymore~~
~~Links that contain channel-like anchors break (http://example.com/#head)~~

AmShaegar13 · 2015-10-10T14:27:21Z

Okay, fixed that too. The only restriction now is that #channels need to be enclosed by spaces or be at the beginning or end of a line.

Do you want me to squash all commits and rebase on master after the final review?

JocelynDelalande · 2015-10-10T14:59:14Z

@AmShaegar13 cool :) just tested :

On feature:

if more than 50 chars, it does nothing if clicked, but still is transformed into link, which is confusing.

On style:

could you create a constants file that will centralize things like max topic length ? (and their justification, like you did in comment mentioning RFC)
avoid fixing style on lines you don't touch to change their content, it's confusing for review reading git history (at least put these changes in dedicated commit), no big deal though

Do you want me to squash all commits and rebase on master after the final review?

Yes please :) it seems to me that they could be one commit.

AmShaegar13 · 2015-10-10T15:09:56Z

if more than 50 chars, it does nothing if clicked, but still is transformed into link, which is confusing.

Well, looks like this depends on the network/irc daemon used. The one I am testing on just drops all characters after the 32nd one. So it will always work. If I set the limit down to 50, it will not work in networks that allow 200 characters. So I'd like to stick with that.

could you create a constants file that will centralize things like max topic length ? (and their justification, like you did in comment mentioning RFC)

I would rather not insert a constant in a regular expression. It makes it even harder to read and understand.

avoid fixing style on lines you don't touch to change their content, it's confusing for review reading git history (at least put these changes in dedicated commit), no big deal though

Sorry, just used search an replace ^^

astorije · 2015-10-12T04:29:42Z

client/js/shout.js

+					.find('.chan.active')
+					.parent('.network')
+					.find(".chan[data-title='" + $(this).data("chan") + "']");
+				if (chan.size() == 1) {


Can you replace with === here please?

astorije · 2015-10-12T04:33:07Z

could you create a constants file that will centralize things like max topic length ? (and their justification, like you did in comment mentioning RFC)

I would rather not insert a constant in a regular expression. It makes it even harder to read and understand.

Agreed, and I'm also against centrally-managed constant files here. Many reasons to that but I have seen that in practice and it didn't work well. Simple is enough, and in the future, I'd like to see this very feature being a plugin (but valued as core feature and enabled by default) so having self-sufficient code is ideal.

I'll give a full final review whenever I can find time to do so, sorry for the delay.

astorije · 2015-10-15T04:10:21Z

client/css/style.css

+	cursor: pointer;
+}
+#chat .msg .inline-channel:hover {
+	opacity: .8;


I noticed existing opacities are floating between .6 and .8 but here I'd prefer .6 to be consistent with existing links on the chat window.

astorije · 2015-10-15T04:17:19Z

Interesting, when you click on the channel name (in Chrome), you get a link border for a second, but not on the nick names. I guess that's one difference between buttons and spans here...

astorije · 2015-10-15T04:20:22Z

Apart from the minor comments I left inline, this looks good to me. Will wait for final amend before 👍.

Getting there, getting there! :-)

AmShaegar13 · 2015-11-22T13:58:35Z

I am sorry. I was very busy. Finally included you remaining points.

astorije · 2015-11-22T22:50:02Z

@AmShaegar13 thanks! To be honest, I had forgotten about this, but hopefully this is almooost ready to go. If you take a look at the Travis report, you'll see that your PR breaks a styling rule. Could you use double quotes instead of single quotes? I'll be happy to merge after that!

astorije · 2015-11-22T22:50:52Z

client/js/shout.js

 			.trigger("msg", [
 				target,
 				data.msg
 			]);

+		var text = msg.find('.text');


Use double quotes here.

astorije · 2015-12-14T02:17:33Z

GitHub doesn't notify commits, so we were not notified you rebased :-)

That's a 👍 for me.
@JocelynDelalande?

JocelynDelalande · 2016-01-06T13:38:14Z

👍 merging, thanks @AmShaegar13 !

astorije · 2016-01-07T05:53:36Z

@JocelynDelalande, you closed instead of merging :-)
Merging now.

made channel names in chat clickable; lets users join channels

AmShaegar13 force-pushed the click_channel branch from 2a9d661 to 164c96e Compare March 29, 2015 10:55

astorije reviewed Sep 26, 2015
View reviewed changes

astorije added enhancement feature labels Sep 26, 2015

astorije self-assigned this Sep 26, 2015

astorije removed the feature label Sep 26, 2015

Xe reviewed Sep 26, 2015
View reviewed changes

astorije reviewed Sep 27, 2015
View reviewed changes

AmShaegar13 force-pushed the click_channel branch 3 times, most recently from 9e145f4 to 7788f69 Compare October 10, 2015 10:59

AmShaegar13 force-pushed the click_channel branch 2 times, most recently from a821d58 to 7c26b04 Compare October 10, 2015 11:05

JocelynDelalande added the second review needed label Oct 10, 2015

AmShaegar13 force-pushed the click_channel branch from 7d69b88 to b3a9946 Compare October 10, 2015 20:44

astorije reviewed Oct 12, 2015
View reviewed changes

astorije reviewed Oct 15, 2015
View reviewed changes

AmShaegar13 force-pushed the click_channel branch from b3a9946 to 359905f Compare November 22, 2015 13:58

astorije reviewed Nov 22, 2015
View reviewed changes

client/js/shout.js

.trigger("msg", [

target,

data.msg

]);

var text = msg.find('.text');

Copy link

Collaborator

astorije Nov 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use double quotes here.

JocelynDelalande mentioned this pull request Dec 1, 2015

Allow simple-quotes strings if it avoids escaping #547

Merged

made channel names in chat clickable; lets users join channels

58d4a2f

AmShaegar13 force-pushed the click_channel branch from 359905f to 58d4a2f Compare December 2, 2015 09:57

JocelynDelalande closed this Jan 6, 2016

astorije reopened this Jan 7, 2016

astorije added a commit that referenced this pull request Jan 7, 2016

Merge pull request #385 from AmShaegar13/click_channel

409936e

made channel names in chat clickable; lets users join channels

astorije merged commit 409936e into erming:master Jan 7, 2016

astorije removed the second review needed label Jan 7, 2016

astorije mentioned this pull request Jan 22, 2016

Feature Request: Click on Channel Links #361

Closed

made channel names in chat clickable; lets users join channels #385

made channel names in chat clickable; lets users join channels #385

Conversation

AmShaegar13 commented Mar 28, 2015

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 commented Sep 26, 2015

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

richrd commented Sep 27, 2015

AmShaegar13 commented Sep 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Sep 27, 2015

AmShaegar13 commented Sep 27, 2015

astorije commented Sep 28, 2015

AmShaegar13 commented Oct 2, 2015

JocelynDelalande commented Oct 4, 2015

astorije commented Oct 4, 2015

JocelynDelalande commented Oct 4, 2015

astorije commented Oct 5, 2015

Xe commented Oct 5, 2015

astorije commented Oct 5, 2015

AmShaegar13 commented Oct 10, 2015

AmShaegar13 commented Oct 10, 2015

JocelynDelalande commented Oct 10, 2015

AmShaegar13 commented Oct 10, 2015

Choose a reason for hiding this comment

astorije commented Oct 12, 2015

Choose a reason for hiding this comment

astorije commented Oct 15, 2015

astorije commented Oct 15, 2015

AmShaegar13 commented Nov 22, 2015

astorije commented Nov 22, 2015

Choose a reason for hiding this comment

astorije commented Dec 14, 2015

JocelynDelalande commented Jan 6, 2016

astorije commented Jan 7, 2016