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

Emoji Syntax Gravatars #105

Conversation

simeonwillbanks
Copy link
Contributor

Fixes #56

  • Propose $simeonwillbanks$ syntax; $ as delimiter
  • Add AvatarFilter test for template method
  • Add GravatarFilter tests for parsing and various context options
  • AvatarFilter defines interface
  • GravatarFilter defines concrete implementation
    • GravatarFilter requires service for converting username to email

Notes

  • Initial implementation for discussion
  • Needs TomDoc

* Propose $simeonwillbanks$ syntax; $ as delimiter
* Add AvatarFilter test for template method
* Add GravatarFilter tests for parsing and various context options
* AvatarFilter defines interface
* GravatarFilter defines concrete implementation
  * GravatarFilter requires service for converting username to email
class Pipeline
class AvatarFilter < Filter

DELIMITER = "$".freeze
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another interesting option is @! which allows reuse of existing @mention helpers.

emojiavatar

@rsanheim
Copy link
Contributor

I realize there was discussion of this back in #56, so I'm a bit late here.

But I'm pretty 👎 at adding this to the pipeline. It feels like adding some strange syntax for something that not be used very often.

@jch
Copy link
Contributor

jch commented Jan 30, 2014

@rsanheim I usually prefer to add filters as a separate gem (e.g. html-pipeline-gravatar). But in this case, I'm open to adding this to the main project because:

  • it doesn't add any external code dependencies
  • avatars are a common and general web feature

Yes, we are depending on the interface of Gravatar, but that's been stable for a long time. The addition of AvatarFilter also allows people to write their own implementation. Whether we want to use this feature on GitHub.com should be a separate discussion for that team. It makes sense to me in the upstream open source gem. Thoughts?

@simeonwillbanks 👍 thanks for the pull. I'll review it after we decide this discussion ^^

@simeonwillbanks
Copy link
Contributor Author

@rsanheim @jch Thanks for the input; I'm cool either way!

@jch
Copy link
Contributor

jch commented Feb 3, 2014

@rsanheim any more thoughts on this?

@rsanheim
Copy link
Contributor

rsanheim commented Feb 4, 2014

@rsanheim any more thoughts on this?

So, I may have a much more focused vision for html-pipeline in general than other maintainers. But I think adding things that we aren't using in github.com directly adds maintenance burden in the long term without much gain. I'm 👎 this change or other similar changes that aren't direct fixes or improvements we can use in .com. The more focused we keep the pipeline, the better.

@bkeepers
Copy link
Contributor

bkeepers commented Feb 4, 2014

I tend to agree with @rsanheim's sentiments. Our open source projects at GitHub that have struggled are the ones that tried to be everything to everyone. I would stay focused on our needs, and encourage people to release their custom extensions as separate gems (like html-pipeline-gravatar).

@jch
Copy link
Contributor

jch commented Feb 5, 2014

The more focused we keep the pipeline, the better.

Fair enough. @simeonwillbanks could you create and maintain a html-pipeline-gravatar gem? We can add a section to the README linking to 3rd party filters.

@jch jch closed this Feb 5, 2014
@simeonwillbanks
Copy link
Contributor Author

@rsanheim and @bkeepers I appreciate your thoughts, and they're sound 😄

@jch Sure!

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.

Emoji syntax gravatars
4 participants