-
Notifications
You must be signed in to change notification settings - Fork 153
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
Added emoji convertion for irc. #26
Conversation
@@ -118,6 +119,13 @@ Bot.prototype.parseText = function(text) { | |||
return '<' + label + '>'; | |||
} | |||
return '<' + command + '>'; | |||
}) | |||
.replace(/\:(\w+)\:/g, function(match) { | |||
var clean = match.replace(/\:/g,''); |
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.
Isn't what you're trying to get from clean
here already passed as the second argument? Since (\w+)
is a regex capture group. See https://github.com/andebor/slack-irc/blob/master/lib/bot.js#L109.
Cool, thanks! Wanna add a test (or modify this)? Can probably borrow a few emojis from here too, https://github.com/wooorm/emoticon/blob/master/data%2Femoticons.json. Could be good to move this to an external NPM package later on as the emoji list grows larger, but for now this'll be a good solution :) |
You're absolutely right. I haven't really done any work with node before, so I'm just learning as I go. I was also wondering what you think should be done if the emoji isn't found in the json file. Should it just be returned raw? or maybe something like:
Maybe it shouldn't show at all? |
Sounds great! I think just returning the raw emoji is a good solution when it isn't found in the JSON-file. Most people probably knows that |
I agree. I added the suggested changes in the latest update. Let me know if you have anymore suggestions! |
Thanks! Added a few more emojis and updated the changelog here. |
This fixes issue #10.
Doesn't support every emoji in slack , but the most common should be covered and new entries can easily be added to emoji.js