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

Adding support to supply the path to the emoji sprite in the emoji pipeline #122

Merged
merged 4 commits into from
Apr 4, 2014
Merged

Adding support to supply the path to the emoji sprite in the emoji pipeline #122

merged 4 commits into from
Apr 4, 2014

Conversation

bradly
Copy link
Contributor

@bradly bradly commented Apr 4, 2014

This was helpful for us, so just seeing if there is interest merging back into master. Let me know if you have any feedback. Thanks!

if context[:asset_path]
context[:asset_path].gsub(":file_name", "#{::CGI.escape(name)}.png")
else
"emoji/#{::CGI.escape(name)}.png"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use File.join instead of /. Thanks!

File.join("emoji", "#{::CGI.escape(name)}.png")

@simeonwillbanks
Copy link
Contributor

Cool configuration option. 👍 What do y'all think @rsanheim and @jch?

@@ -12,6 +12,7 @@ class Pipeline
#
# Context:
# :asset_root (required) - base url to link to emoji sprite
# :asset_path (optional) - url path to link to emoji sprite. :file_name can be used as a placeholder for the sprite file name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe explain here that if left unspecified, the asset path will be "emoji/:file_name"

@jch
Copy link
Contributor

jch commented Apr 4, 2014

👍 nice change. One of us will be happy to merge this and cut a new release after the small requested changes above

@bradly
Copy link
Contributor Author

bradly commented Apr 4, 2014

Thanks, guys!

I'll get those changes up shortly.

@simeonwillbanks
Copy link
Contributor

Looking good 👏 I'll :shipit:

simeonwillbanks pushed a commit that referenced this pull request Apr 4, 2014
Adding support to supply the path to the emoji sprite in the emoji pipeline
@simeonwillbanks simeonwillbanks merged commit 16ef9e0 into gjtorikian:master Apr 4, 2014
@bradly
Copy link
Contributor Author

bradly commented Apr 4, 2014

Sweet, thanks for the help!

One thing I noticed... You are using File.join to join URI segments, but File.join uses the OS's file system delimiter which could be different from '/', especially if someone is developing on Windows. URI has URI.join but it doesn't work for relative URI's or protocal-less hosts like "//example.com". Not sure if this is worth worrying about, but if it is I could take a crack at it.

@jch
Copy link
Contributor

jch commented Apr 4, 2014

@bradly don't know much about that because that was before I started maintaining this project. Would love to hear your findings if you do look into it though. ;)

@simeonwillbanks
Copy link
Contributor

I'll continue with the release...

@simeonwillbanks
Copy link
Contributor

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.

3 participants