Fix Gemoji with latest gem #347

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

sfcgeorge commented Dec 1, 2012

Fixes #340. The latest Gemoji gem's recommended usage is to serve the images from Public rather than Assets because the hundreds of images slow down asset precompilation.

This commit updates the gem, and changes the helper to use the public path.

  • I have hardcoded the /images/emoji bit of the path, I'm not sure if this is the best way or if there is a rails helper to do it. Would the public directory ever move? Would people ever put images in a folder other than images? I've only used the asset pipeline before so I'm not sure if there are any gotchas here to be aware of.
  • There is a rake task that needs adding to the app's rakefile and running to copy the images from Gemoji to public/images. See the Gemoji readme for info. It may be worthwhile making the rake task run automatically on Forem install, not sure how that would work. Currently having to edit the Rakefile and run a task doesn't seem ideal, but it might not be the end of the world.
  • I had to add a require for Gemoji and change the class slightly as Rails wasn't finding it, I'm not sure why, perhaps the gem changed its structure slightly or I'm missing something. What I've done works, I'm just not certain it's the Best Way.
Contributor

parndt commented Dec 1, 2012

@radar given this should we stick with gemoji?

@parndt parndt commented on the diff Dec 1, 2012

app/helpers/forem/application_helper.rb
h(content).to_str.gsub(/:([a-z0-9\+\-_]+):/) do |match|
- if Emoji.names.include?($1)
- '<img alt="' + $1 + '" height="20" src="' + asset_path("emoji/#{$1}.png") + '" style="vertical-align:middle" width="20" />'
+ if ::Emoji.names.include?($1)
+ '<img alt="' + $1 + '" height="20" src="' + "/images/emoji/#{$1}.png" + '" style="vertical-align:middle" width="20" />'
else
@parndt

parndt Dec 1, 2012

Contributor

This will break support for deployment to sub URI

@sfcgeorge

sfcgeorge Dec 2, 2012

Contributor

Ah yes of course. Is there a Rails helper to return the public images path? I searched without much luck, images_path was suggested years ago but that now returns /assets.

Contributor

parndt commented Dec 1, 2012

To me this seems like a step backward on Gemoji's part

Contributor

sfcgeorge commented Dec 2, 2012

Rebased for cleaner commit history.

The tone of the Gemoji project seems quite closed off, as suggested by disabled issues and a code comment saying pull requests to do with this change would be ignored without comment. The Readme also doesn't make it entirely clear the direction they are trying to go in. I did look for alternative Emoji libraries but couldn't find anything as active, otherwise changing library might be an appealing option. As an aside, most of the Emoji are screenshots of Apple's Emoji font - thus copyright Apple - do we know if it is legal to do that? I doubt Apple has given away its Emoji font with an OS license, though I'm no lawyer.

Contributor

parndt commented Dec 6, 2012

Not sure how I feel about the project given how closed off it is and given the commit messages don't make things any easier to understand what has changed.

Contributor

parndt commented Dec 6, 2012

Okay after going through the changes it seems pretty reasonable and these are the things we can do:

  • document the rake gemoji take
  • add the Emoji to the assets paths like so:
config.assets.paths << Emoji.images_path

Then have them compiled to public on deploy.

config.assets.precompile << "emoji/*.png"

The last option is my favourite. It means this pull request will have to completely change though.

We will have to figure out what config.assets.precompile means for Forem. Probably we just have to create an initializer or something.

Contributor

parndt commented Apr 27, 2013

So what should we do?

Contributor

sfcgeorge commented Apr 27, 2013

Phantom Open Emoji got funded , so soon we will have an open source legal alternative to Gemoji that we can use. The way we use it is still up for debate, but it's the way forward.

radar closed this Mar 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment