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

Feature/emoji strip #56

Merged
merged 4 commits into from
Jul 23, 2017
Merged

Feature/emoji strip #56

merged 4 commits into from
Jul 23, 2017

Conversation

smeijer
Copy link

@smeijer smeijer commented Jul 23, 2017

Closes #17

The following functionality is added:

Emoji.replace

emoji.replace('Where did this πŸ‘©β€β€οΈβ€πŸ’‹β€πŸ‘© happen?', '', true);
// Β» Where did this happen?

emoji.replace('There is no ⚠ on my hard drive', x => x.key);
// Β» There is no warning on my hard drive

And some things can't be done; as discussed in #17

// this ain't 3 emoji's, it's an unsupported complex emoji
emoji.replace('Some πŸ•β€οΈβ€πŸ’‹β€β˜• emoji', ''); 
// Β» Some πŸ•β€οΈβ€πŸ’‹β€β˜• emoji');

Emoji.strip

Emoji.strip is a shortcut for replace(str, '', true);

emoji.strip('Host: eseaps001 Addr: 10.XX.XX.XX: - ⚠️ 〰️ 〰️ low disk space');
// Β» Host: eseaps001 Addr: 10.XX.XX.XX: - low disk space

@smeijer
Copy link
Author

smeijer commented Jul 23, 2017

At first I did implement a much simpler version; something like:

var newStr = Emoji.unemojify(str);
var cleaned = Emoji.emojify(str, () => '', () => '');
return cleaned.trim();

It seemed to do it's thing, but the double iteration didn't feel right. Also, I think :smile: should be left alone, as it's a string, and no emoji. When someone needs to replace that as well, they should use the format parameter of the emojify method.

So I've decided to implement the function in a similar way as the unemojify. Loop over the words, clean spaces when wanted and necessary, and return the string.

@@ -10,6 +10,12 @@ var emojiByName = require('./emoji.json');
var emojiNameRegex = /:([a-zA-Z0-9_\-\+]+):/g;

/**
* regex to trim whitespace
* use instead of String.prototype.trim() for IE8 supprt
Copy link
Owner

Choose a reason for hiding this comment

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

typo: support

Copy link
Author

Choose a reason for hiding this comment

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

Oops. And you've already merged the thing 😞

Copy link
Owner

Choose a reason for hiding this comment

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

@smeijer I fixed it myself in f41a376 since it was only a small change 😁

Copy link
Author

Choose a reason for hiding this comment

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

Awesome 😁

@@ -27,6 +27,8 @@ emoji.find('πŸ•'); // Find the `pizza` emoji, and returns `({ emoji: 'πŸ•', ke
emoji.find('pizza'); // Find the `pizza` emoji, and returns `({ emoji: 'πŸ•', key: 'pizza' })`;
emoji.hasEmoji('πŸ•'); // Validate if this library knows an emoji like `πŸ•`
emoji.hasEmoji('pizza'); // Validate if this library knowns a emoji with the name `pizza`
emoji.strip('⚠️ 〰️ 〰️ low disk space'); // Strips the string from emoji's, in this case returns: "low disk space".
Copy link
Owner

Choose a reason for hiding this comment

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

are the 〰️ characters getting stripped too? they are not emoji, right?

Copy link
Owner

Choose a reason for hiding this comment

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

they actually are emoji, so this behavior is fine.

Copy link
Author

Choose a reason for hiding this comment

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

wavy_dash, it's not a ~, which of course isn't being replaced.

@omnidan
Copy link
Owner

omnidan commented Jul 23, 2017

thank you so much - looks good 😁

@omnidan omnidan closed this Jul 23, 2017
@omnidan omnidan reopened this Jul 23, 2017
@omnidan omnidan merged commit 0862eb0 into omnidan:master Jul 23, 2017
@omnidan
Copy link
Owner

omnidan commented Jul 23, 2017

released 1.8.1 with this PR merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants