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

Add support for GitHub Emojis #71

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

Ry-DS
Copy link

@Ry-DS Ry-DS commented Jan 5, 2021

Closes #67
Works well for most cases:
image
image

How the customAliases were found:

// GitHub exclusive emojis - not part of unicode standard.
// These were found by singling out emojis without `/unicode` in their name from: https://api.github.com/emojis

Issues:

  • :octocat::octocat: doesn't work because :o turns to :open_mouth. There are some more emojis like this. Let me know if you have a good solution to this... Solved!

@enzoferey
Copy link
Collaborator

Hi @Ry-DS ! Thanks a lot for the PR.

:octocat: :octocat: doesn't work because :o turns to :open_mouth. There are some more emojis like this. Let me know if you have a good solution to this...

Based on https://github.com/tommoor/react-emoji-render/blob/master/src/renderer.js#L76, if we have the github aliases in our dataset, we should not have that issue. I understand this is an edge case as Github emojis are not part of the unicode standard and we can't really have entries in that file without a matching unicode emoji, but I think your customAliases approach is very interesting for this use case. Maybe if you add another condition at https://github.com/tommoor/react-emoji-render/blob/master/src/renderer.js#L84 checking if there is a match with some of the custom aliases it's enough to make it work without any further modification in the replaceAliases function.

@enzoferey
Copy link
Collaborator

@Ry-DS any chance you pull this off to an end ?

@Ry-DS
Copy link
Author

Ry-DS commented Jul 19, 2021

@enzoferey hey! Thanks for the reminder. I did a quick commit which I think fixes it, definitely take a look and confirm if you think its an ok solution. If so, I plan to polish it up with comments later today and it should be g2g.

@Ry-DS Ry-DS marked this pull request as ready for review July 19, 2021 03:18
@Ry-DS
Copy link
Author

Ry-DS commented Aug 26, 2021

@enzoferey just bumping this, let me know if you need anything else from my end (I should be a little more active to any requests this and next month) :)

@enzoferey
Copy link
Collaborator

Hey @Ry-DS , thanks for the follow up, this is far from a priority for me 😅

Reading again the comments above and your code, I had something very specific in mind that should reduce the complexity of the chances introduced and enable future similar use cases to integrate in the same way. Quoting my previous message:

Maybe if you add another condition at https://github.com/tommoor/react-emoji-render/blob/master/src/renderer.js#L84 checking if there is a match with some of the custom aliases it's enough to make it work without any further modification in the replaceAliases function.

So far you have changed a lot about how the replacing and rendering algorithms work, but I think just introducing a single check on the customAliases props could solve it all.

Also, we are introducing a lot of useless tests on the different vendors that only affect the Github component, we should improve that don't you think so ?

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.

Add support for Github emojis
2 participants