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

Add auto-prepend behaviour for unprefixed channel names #2289

Merged
merged 1 commit into from
Mar 29, 2018

Conversation

lol768
Copy link
Contributor

@lol768 lol768 commented Mar 24, 2018

This change adds behaviour to automatically prefix channel names passed in via the "?join=x,y,z" query string/search parameter which do not appear to include an appropriate channel symbol. For example, channel becomes #channel but #channel and ~channel are left alone.

Known limitation: Unicode characters at the start of the channel name will not be considered to match /^\w/. These will not be auto-prefixed.

If this is not the correct place to implement this functionality, please let me know where it would be better suited.


if (key === "join") {
let channels = value.split(",");
channels = _.map(channels, (c) => {
Copy link
Member

Choose a reason for hiding this comment

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

Use a native method instead of bringing lodash here (we don't use it on the client).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed this and squashed the commit.

Copy link
Member

@xPaw xPaw left a comment

Choose a reason for hiding this comment

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

Works fine.


return c;
});
value = channels.join(",");
Copy link
Member

Choose a reason for hiding this comment

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

You can chain value = split.map.join.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout, changed

Copy link
Member

Choose a reason for hiding this comment

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

Did you push the change? Doesn't appear to be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would seem I didn't git add before amending the commit. Fixed now in 25dee77

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Mar 24, 2018
This change adds behaviour to automatically prefix channel names passed in via the "?join=x,y,z" query string/search parameter which do not appear to include an appropriate channel symbol.
@MaxLeiter
Copy link
Member

👍 from me

@xPaw
Copy link
Member

xPaw commented Mar 25, 2018

@MaxLeiter You can review/approve the PR from files changed tab.

@astorije
Copy link
Member

Known limitation: Unicode characters at the start of the channel name will not be considered to match /^\w/. These will not be auto-prefixed.

Would it work if, instead of adding # when the string starts with \w, this add # if the string does not start with #, &, or any other chan types (at the moment we only parse & and # in messages, but I wouldn't be surprised if we had similar logic in more than one place).

@lol768
Copy link
Contributor Author

lol768 commented Mar 27, 2018

Would it work if, instead of adding # when the string starts with \w, this add # if the string does not start with #, &, or any other chan types (at the moment we only parse & and # in messages, but I wouldn't be surprised if we had similar logic in more than one place).

We could certainly make the decision to limit it to those characters (and have some centralised logic somewhere for fetching/checking for channel type characters).

@astorije
Copy link
Member

What do @xPaw and/or @McInkay prefer?

@AlMcKinlay
Copy link
Member

Ideally we should use chantypes from the server, no?

@xPaw
Copy link
Member

xPaw commented Mar 28, 2018

We don't know them before connecting.

@AlMcKinlay
Copy link
Member

Hmmm, but this is just for joining channels, isn't it? So we should already be on the network.

@xPaw
Copy link
Member

xPaw commented Mar 28, 2018

This is joining from the connect window. It would be way more complicated to wait for server to connect then fix channels.

@AlMcKinlay
Copy link
Member

In that case, I'd prefer the current solution than just using # and &.

@astorije
Copy link
Member

Alright then. I still think we should check for chan prefixes (and we can probably do it statically by including chan types that might not be on this specific server), but I guess this is the most conservative approach.

@astorije astorije added this to the 3.0.0 milestone Mar 29, 2018
@astorije
Copy link
Member

astorije commented Mar 29, 2018

(I'm merging as-is despite the CI failures, the linting issue reported is not caused by this PR and was fixed on master)

@astorije astorije merged commit fe08547 into thelounge:master Mar 29, 2018
@astorije
Copy link
Member

Thanks a lot @lol768 and congrats on your first merged PR!! :) 🎉

@astorije
Copy link
Member

Hey @lol768, we have sticker packs for our contributors now!
If you're interested, go fill the form at https://goo.gl/forms/f5usqAEp5DWoeXC92 🙂

@lol768
Copy link
Contributor Author

lol768 commented Apr 28, 2018

Awesome stuff @astorije 😄 - filled in the form!

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

5 participants