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.

Sign up for GitHub

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

Jump to bottom

[Emojis in Markdown] #849

Closed
wants to merge 2 commits into from
Closed

[Emojis in Markdown] #849

wants to merge 2 commits into from

Conversation

juanmanuelramallo
Copy link
Contributor

@juanmanuelramallo juanmanuelramallo commented Oct 8, 2018 •
edited
Loading

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

  • Added the ability to add emojis using colons :joy: :tada:
  • The emoji conversion is added to the markdown parser labor
  • When no emoji is found with the alias, same text is returned
  • Used gemoji gem to handle emojis and unicode mapping
  • No assets added (no emoji images), so this will only be supported when the browser supports emojis (https://blog.getemoji.com/post/57054354336/which-browsers-support-emoji)
  • Tests updated

Related Tickets & Documents

#203

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Not applicable

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

peasy

@CLAassistant
Copy link

CLAassistant commented Oct 8, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@juanmanuelramallo juanmanuelramallo force-pushed the feature/emojis-in-comments-using-colon branch 2 times, most recently from 6ac3777 to c5d2854 Compare October 8, 2018 03:00
@juanmanuelramallo
Copy link
Contributor Author

Need help

Any suggestions on how to make codeclimate pass? @benhalpern @jessleenyc

@benhalpern
Copy link
Contributor

I think I like this, but it's the kind of thing I have mixed feelings about. Mostly because I haven't really made use of this functionality myself. It feels like markdown maybe shouldn't care about this.

Although. It would be a nice shortcut for custom emojis for dev.to, which I like.

I think I'm in favor but I'm going to give it some thought.

Regarding codeclimate passing, you could extract some of the private methods here into other objects. For example, user_link_if_exists and similar methods could be extracted into separate moules like MentionConverter etc. or something of that nature and dumped in labor folder. 🙂

@juanmanuelramallo
[Emojis in Markdown]
f0528cd 
- Added the ability to add emojis using colons 😂 🎉
- The emoji conversion is added to the markdown parser labor
- When no emoji is found with the alias, same text is returned
- No assets added (no emoji images), so this will only be supported when the browser supports emojis (https://blog.getemoji.com/post/57054354336/which-browsers-support-emoji)
@juanmanuelramallo juanmanuelramallo force-pushed the feature/emojis-in-comments-using-colon branch from 06ff99c to 394d2c5 Compare October 16, 2018 01:18
@juanmanuelramallo
Copy link
Contributor Author

Please have a look when possible, I just refactored it to make code climate pass by creating a emoji converter class (didn't want to modify any method in the markdown parse labor not related to this feature), and let me know what you think!
cc @benhalpern
Thanks!

@juanmanuelramallo
[Refactoring]
641db25 
- Moved emoji conversion logic out of markdown parser labor
- Added its own spec
@juanmanuelramallo juanmanuelramallo force-pushed the feature/emojis-in-comments-using-colon branch from 394d2c5 to 641db25 Compare October 16, 2018 03:36
@weekly-digest weekly-digest bot mentioned this pull request Oct 29, 2018
Closed
@maestromac maestromac self-assigned this Nov 27, 2018
@maestromac maestromac removed their assignment Jan 16, 2019
@benhalpern
Copy link
Contributor

After a lot of long deliberation, I'm down to make this happen.

@juanmanuelramallo would you mind updating this to make it work alongside some of the changes we've made since?

@juanmanuelramallo
Copy link
Contributor Author

Sure thing @benhalpern!
I'll rebase with master and fix the conflicts.
Any other changes in here I'd need to address?

@juanmanuelramallo juanmanuelramallo mentioned this pull request Jan 25, 2019
Merged
7 tasks
@juanmanuelramallo
Copy link
Contributor Author

Updated pull request in here #1653

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
PR: unreviewed bot applied label for PR's with no review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@juanmanuelramallo @CLAassistant @benhalpern @maestromac