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

Get emoji data from native #250

Merged
merged 21 commits into from
Mar 23, 2019

Conversation

pederjohnsen
Copy link

@pederjohnsen pederjohnsen commented Nov 29, 2018

Adds function to get emoji data from native unicode emoji

This resolves #16 and resolves #193

[edit]
Thought I had already fixed it but seems the issues with some emojis as mentioned in #16 (comment) are still present here too.. If anyone has any thoughts as to why and how to fix it let me know 🙃

@ifiokjr
Copy link

ifiokjr commented Feb 28, 2019

This looks very helpful. Any chance it gets merged soon?

@nolanlawson
Copy link

It seems based on discussion in #16 that this still needs some more work?

@pederjohnsen
Copy link
Author

Yes unfortunately it still needs more work, I've not got the time to revisit this at the moment, but once I have (unless someone else has had a look by then) I'll post an update in here 👌

@pederjohnsen
Copy link
Author

@nolanlawson I've updated the code and it seems to work correctly for all emojis now...

This also includes an update to nimble-emoji-index.js which returns search results for the skin tone you've picked in the picker. If I remember correctly this isn't required for the getEmojiDataFromNative to work. But was another thing I needed for an app I'm working on. I could pull this out and create a separate PR if wanted 👍

Copy link

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

I've taken a quick look and provided some feedback. I think indeed that breaking it up into two PRs may be a good idea. If you also have a chance to add some Jest tests that would be grand. :)

src/utils/index.js Outdated Show resolved Hide resolved
src/utils/index.js Outdated Show resolved Hide resolved
src/utils/index.js Outdated Show resolved Hide resolved
src/utils/index.js Outdated Show resolved Hide resolved
src/utils/index.js Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
@pederjohnsen
Copy link
Author

Thanks for reviewing, I'll try and get on this over the weekend 👍

This was referenced Mar 23, 2019
@pederjohnsen
Copy link
Author

@nolanlawson I've updated the code and added some tests, let me know if you find anything else 👌

@nolanlawson
Copy link

This LGTM, I think the only thing missing is that it would be great to have a Jest test for this, since this is a pretty self-contained little util function. I don't think it should necessarily block this PR from being merged, though.

@pederjohnsen
Copy link
Author

ab2f600 ? 🤔

@nolanlawson
Copy link

Oops, my bad, did not see the tests. Everything LGTM!

@nolanlawson nolanlawson merged commit 7d8e245 into missive:master Mar 23, 2019
@pederjohnsen
Copy link
Author

Thanks :) 🎉

@pederjohnsen pederjohnsen deleted the get_emoji_data_from_native branch March 23, 2019 22:09
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.

Need to convert native emoji in a string to colons Get Emoji from its unicode
4 participants