Supports Unemojify. #41
Conversation
omnidan
left a comment
omnidan
left a comment
There was a problem hiding this comment.
Thanks for the pull request! 😁 I added some comments. Please have a look at them and let me know if anything is unclear!
| *.txt | ||
| test.js | ||
| coverage | ||
| play.js No newline at end of file |
There was a problem hiding this comment.
why is this added to the gitignore file? if you just used this file to test out the library, please remove it from the pull request.
There was a problem hiding this comment.
Pushed this by mistake. Will remove 👍
| @@ -1,5 +1,6 @@ | |||
| /*jslint node: true*/ | |||
| require('string.prototype.codepointat'); | |||
| var toArray = require('lodash/toArray'); | |||
There was a problem hiding this comment.
I don't really want to pull in all of lodash as a dependency just for a way to convert something to an array - surely there must be a way to accomplish the same behaviour without lodash?
There was a problem hiding this comment.
So the issue is with some complex emojis.
let's take an example.
var str = '👩❤️💋👩';
/* using lodash's toArray */
toArray(str); // outputs [ '👩❤️💋👩' ] (length: 1)
/* using Array.from() */
Array.from(str); // outputs: [ '👩', '', '❤', '️', '', '💋', '', '👩' ] (length : 8)
/* using string split() */
str.split(''); // outputs [ '�', '�', '', '❤', '️', '', '�', '�', '', '�', '�' ] (length : 11)
I searched quite a bit to achieve this without lodash but no luck 👎 .
There was a problem hiding this comment.
@shivkanthb I see, in that case, it might be best to just use https://www.npmjs.com/package/lodash.toarray (so that we don't have to have all of lodash as dependency)
| } | ||
|
|
||
| Object.prototype.getKeyByValue = function(value) { | ||
| for(var prop in this) { |
There was a problem hiding this comment.
please stick to the style in the rest of the library (space after for and if)
There was a problem hiding this comment.
Oops. Missed that. Will do! 👍
| it("should leave unknown emoji", function () { | ||
| var coffee = emoji.unemojify('I ⭐️ :another_one: 🥕'); | ||
| should.exist(coffee); | ||
| coffee.should.be.exactly('I :star: :another_one: 🥕'); |
There was a problem hiding this comment.
shouldn't this un-emojify the carrot emoji?
There was a problem hiding this comment.
Right now it only unemojifies the ones present in emoji.json. If carrot is added to that, this will be resolved automatically.
|
@shivkanthb did you manage to review the comments and push your changes yet? |
|
@shivkanthb thanks! can you also document this feature in the README.md? |
|
Readme updated 👍 |
|
@shivkanthb looks good now - thank you so much 😁 👌 |
|
released |
|
@shivkanthb actually, this PR ended up breaking yarn (#45 and yarnpkg/yarn#3851), so I had to revert it. Someone came up with a possible fix in #46 |
…fy"" This reverts commit bfb2e78.
2 participants
Usage:
Outputs:
Fixes issue #36