No info in README.md about replace(string, callback)

[adam-lynch]

Use case: email. I need to inline the styles suggested in gh-pages;

<img title=':fr:' alt=':fr:' class='emoji' src='images/emoji/fr.png' align='absmiddle' style='width:1.5em;height:1.5em;display:inline-block;margin-bottom:-0.25em;' />

Options:

1. Allow the user to provide an optional style value.
2. Allow the user to pass an object (attributes to values) which are taken and added to the element. Not sure what to do here though if they specify an attribute this package adds itself. Maybe append, maybe not.
3. Allow the user to pass a callback function which is called once the element is created. It could get passed some details; the emoji found, the named emoji which it will be replaced with, and the image URL.

Option 3 is definitely the best option. The problem is what arguments should the callback have and what should the user return.

Ideally, for browser users, getting passed the HTML element would be best;

emojify.setConfig({
    onReplaceCallback: function(element, args){
        if(args.type === 'emoticon') element.class += ' emoticion-used';
        return element;
    })
});

I'm using this with Node myself. For Node users, I guess all you can give them is the string / attributes. It would be terrible to make this inconsistent though. For the sake of consistency between browser & Node, how about this for both?

emoji.replace(text, function(attributes){
    attributes.style = 'background: blue;';
    attributes.title += ' emoji';
    return attributes;
});

And only allow it in emojify.replace until the rest of the features are made compatible with Node. (#50).

[hassankhan]

It's an interesting idea, but I'm not quite sure if there's that much of a need for it. You said you needed it for emails, maybe you could use a style inliner instead? I know of at least one

[adam-lynch]

The inliners out there are really not up to scratch. 99% of them depend on Juice which has fatal bugs on Windows.

A lot of libraries that do stuff like this allow callbacks, etc. to modify the element / add classes, etc. See https://github.com/ichord/At.js/wiki/Callbacks, especially the last three. I don't have a use case for adding a class right now, but I think it's very likely that someone might one to add classes / other attributes.

[adam-lynch]

I carried on reading through my notifications and already, I've found someone asking for a similar thing with another project; twitter/twitter-text-js#134. This library parses mentions (ie. @someones-name) and sticks them in anchor, etc.

[hassankhan]

I always thought Premailer was quite good, it's what I use at work, sure it doesn't suit your needs?

[adam-lynch]

I think we didn't go with that just because it's not Node. Also see https://github.com/Automattic/juice/pull/35#issuecomment-47364321 (and there are more issues with other ones). Even if the swig mailer one fixed it, then swig itself doesn't support Windows properly IMO.

I think it comes down to this, any plugin that puts HTML on a page should allow users some control over those HTML elements. Usually it's done with callback hooks or something like that. I've needed to do it with almost every library I've used which injected HTML, for example: validators that inject validation messages, datepickers, etc. Sure my use case is for a style attribute, but another person might need a class, another a data attribute, etc. I don't see any good reason not to be support it.

As of right now, after the replacing, I use regex to grab the image's with the emoji class and stick on the style attribute but that isn't nice at all.

[4ver]

+1 for this. An api like marked (https://github.com/chjj/marked#renderer) would be great. They let you provide a custom renderer which contains optional functions that are called for each type of element. You're given the text (and other relevant arguments) and return a string of the html that will be output.

renderer = function (emojiName, imagePath) {
    return '<img src="' + imagePath + '" alt="' + emojiName + '" data-whatever="something"/>'
}

[hassankhan]

Actually .... I've just remembered, you can use replace(htmlString, replacer), where replacer is a callback function (see defaultReplacer in the source).

[adam-lynch]

Oh @hassankhan, this is great if correct. These kind of things really need to be added to the readme & gh-pages though.

[hassankhan]

I know, you're totally right, the readme is in dire need of a good rewrite. Hope that helps with your use case now, though :smile:

[hassankhan]

Leaving it open so I remember to update the readme

[4ver]

Excellent. Would be good to have the path or config passed as well. Thanks!

[hassankhan]

Well, it currently uses the img_dir value passed in the config object. Is that what you're referring to?

[4ver]

That the img_dir variable should be passed through to the replacer function.

[hassankhan]

I don't think it needs to be, necessarily. Any specific reason why, other than good practice?

[4ver]

Good practice I guess. I'd expect to be passed all of the variables needed to reproduce what the default replacer returns. Otherwise, as a user you'd have to keep track of the config outside of the module. Is not a big deal though.

[adam-lynch]

Good practice definitely. If I pass dir/dir/ for example, you have logic that handlings that trailing slash, then I have to duplicate that logic in my replacer function and always make sure it's in sync with yours because I might only want to pass the custom replacer in some cases and reply on the default in others.

[adam-lynch]

Also, maybe it should be passed an object as a parameter instead. There are already two parameters and we're saying it should have the base URL there now too. It's normally said not to go past three paramters. If it was an object, you could easily add more in the future without actually having to add an actual parameter to the method signature / API.

[hassankhan]

Yeah that was one of my concerns.

[adam-lynch]

Ok, this is now much cleaner for my use case :smile:;

    renderEmoji: (text) =>
        emojiReplacerArgs = [text]
        emojiReplacerArgs.push @customEmojiReplacer if @inlineEmojiStyle

        return emoji.replace.apply this, emojiReplacerArgs

    customEmojiReplacer: (emoji, name) =>
        return "<img title='#{emoji}' alt='#{emoji}' class='emoji' src='#{@emojiBaseUrl}/#{name}.png' align='absmiddle' style='width:1.5em;height:1.5em;display:inline-block;margin-bottom:-0.25em;'/>"

(CoffeeScript)

[4ver]

Joob! The config would need to be a property of this though.

[adam-lynch]

Don't forget to document that when a short emoji is matched (e.g. :)), that the arguments will be :) and smile, instead of :smile: and smile.

But really all three should be passed; the emoji found (:)), the name (smile) and the emoji (:smile:). Because of the inconsistency here, the replacer above now doesn't work; it needs to ignore the emoji parameter;

customEmojiReplacer: (emoji, name) =>
        return "<img title=':#{name}:' alt=':#{name}:' class='emoji' src='#{@emojiBaseUrl}/#{name}.png' align='absmiddle' style='width:1.5em;height:1.5em;display:inline-block;margin-bottom:-0.25em;'/>"

[adam-lynch]

@4ver

The config would need to be a property of this though.

Ignore those kind of details which are just specific to my example implementation. Like, my example also depends on @inlineEmojiStyle which is defined elsewhere.

[4ver]

Roger that!

[hassankhan]

I don't agree with the need for all three to be passed (i.e. the emoji found, the name and the emoji). Right now you get the emoji/emoticon pattern that was matched, and the corresponding image name.

Having the image directory passed through can/maybe should be done, but as it currently stands you can retrieve it from defaultConfig anyway, right?

[hassankhan]

I've updated README.md, mind having a quick read, see if I've missed anything out?

[adam-lynch]

Having the image directory passed through can/maybe should be done, but as it currently stands you can retrieve it from defaultConfig anyway, right?

Can you give an example?

The README looks good except you should probably mention that run can't be used in Node / only replace can be.

[hassankhan]

function newReplacer(emoji, name) {
    return emojify.defaultConfig.emojify_tag_type; // Will be whatever you set it to with `emojify.setConfig()`
}

Thanks, I'll update README.md again

[adam-lynch]

Maybe .replace deserves it's own proper section in the readme.

[adam-lynch]

return emojify.defaultConfig.emojify_tag_type; // Will be whatever you set it to with `emojify.setConfig()`

defaultConfig is a misleading name then :/.

Also what if I don't pass an image base URL, will the default be in that property?

[hassankhan]

If you don't pass an image base URL, the default is 'images/emoji', just like it says on the readme. I do agree, though, defaultConfig is misleading. defaultConfig should be the defaults, and there should probably be a config object that is a copy of it, which gets overwritten by emojify.setConfig()

[adam-lynch]

If you don't pass an image base URL, the default is 'images/emoji', just like it says on the readme.

Yeah but it's not accessible via defaultConfig like anyone would assume, plus if it ever changed then any user's code assuming it's images/emoji would break. As @4ver said, "I'd expect to be passed all of the variables needed to reproduce what the default replacer returns." That is just common practice. I feel like a lot of people would be confused using this library, things aren't obvious / as expected / like other libraries.

---

The readme could still be better I think. It would help a lot in this respect.

Now type in an emoji keyword in your HTML, for example :smile: Now run emojify using emojify.run(). To exclude tags from being emojified, add no-emojify to their class attributes.

You can optionally pass an object to emojify.run() to restrict the emojification to that object only: emojify.run(document.getElementById('my-element'))

You can also use emojify.replace() method to emojify a string directly

Documenting methods in this way means someone has to read through big blocks of text to see what they need to do / use. Why not have a sections (with their own big headings) for:

• How to add it to your project; script tag, bower, node
• .setConfig
• .run
• .replace
• Full usage example

It's important to clearly document each method's parameters and return values. What some projects do is generate some of their readme from code comments. I also think it's very important that the readme and gh-pages are very consistent; then if you change one, you should update the other too. Or, really simplify one (let's say gh-pages) to a small description (maybe with a live textarea example) and then link to the "documentation" (the readme).

(Plus, clearly label what works on Node and what doesn't)

[hassankhan]

Updated readme in 81573459441ed0d417ccb50823695083d76ec445. Better? I haven't gotten around to the GitHub Pages yet, I'm going to just leave it as a live sample, and make it point to the repo for everything else.

[adam-lynch]

Better. How about something like this for parameters:

run([element])

This only works in the browser

Parameters

• element - Optional HTML element to restrict the emojification to.

[hassankhan]

Nice!! Looks much better :smile:

[hassankhan]

Pushed another update, mind have a look, please? :smile:

[adam-lynch]

The difference between ### and #### isn't very obvious (like the method name & "Parameters") so maybe change that to one of the following:

• ### and ##### /
• ### and **heading**
• put a horizontal line (---) between each method.

Change the first "Usage" heading to "API". Makes more sense to me, plus it's confusing since you have a "usage" section inside each method too anyway.

Small thing: npm should be lowercase. I saw they said it one of their docs once.

---

I just saw this now:

To exclude tags from being emojified, add no-emojify to their class attributes.

I so wish I could do this in Node! :smile:

[hassankhan]

Ok, pushed again.

I so wish I could do this in Node! :smile:

Haha, well I'm planning to remove that in favour of a blacklist option which would have that as a default instead.

[hassankhan]

Guess this can safely be closed now :+1: