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

New country emojis #41

Merged
merged 29 commits into from
Jan 28, 2016
Merged

New country emojis #41

merged 29 commits into from
Jan 28, 2016

Conversation

jasonrudolph
Copy link
Collaborator

As part of #40, and following in the footsteps of #25 and #26, this pull request aims to add all of the country flags.

TODO

  • Proof-of-concept: Add countries "AD" (Andorra 🇦🇩) through "AZ" (Azerbaijan 🇦🇿)
  • Get feedback from @muan on the approach
  • Add remaining countries: "BA" (Bosnia & Herzegovina 🇧🇦) through "ZW" (Zimbabwe 🇿🇼)

@muan: How does this look so far? What should I change about the approach before adding the remaining countries?

@muan
Copy link
Owner

muan commented Jan 27, 2016

✨ thank you so much! just one thing to note here: they should be ordered like OSX (can be found in the native emoji picker):

image

I have yet to find a text version of this order though. Easiest way would be to insert them one by one using the picker 😂.

And I think we can update the keys programmatically in one go once a category is finished. 👍 I also just added test on master for counting each category has the right number of emojis:

image

jasonrudolph added a commit to jasonrudolph/emojilib that referenced this pull request Jan 27, 2016
Ensure that the first 16 countries are in the proper order as described
in muan#41 (comment).
@jasonrudolph
Copy link
Collaborator Author

@muan: Thanks for the quick feedback!

just one thing to note here: they should be ordered like OSX (can be found in the native emoji picker)

As of 2a4451c, the first 16 countries (i.e., all the ones that start with "A") are in the order shown in the OS X emoji picker:

screen shot 2016-01-26 at 9 16 28 pm

With that update in place, how is this looking so far?

"char": "🇹🇩",
"category": "flags"
},
"chile": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of the country flags use the ISO 3166-2 country code as the emoji code. Chile's ISO code is "cl", but "cl" is already used by one of the previously-existing emojis, so I'm proposing that we use "chile" as the emoji code here.

According to Wikipedia [1], it seems like DRC is probably a better
emoji code for Democratic Republic of the Congo.

[1]: https://en.wikipedia.org/wiki/Democratic_Republic_of_the_Congo
"char": "🇨🇬",
"category": "flags"
},
"drc": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of the country flags use the ISO 3166-2 country code as the emoji code. The ISO code to use here would be "cd", but "cd" is already used by one of the previously-existing emojis, so I'm proposing that we use "drc" as the emoji code here, since that seems to be a commonly-used abbreviation.

@muan
Copy link
Owner

muan commented Jan 27, 2016

All seems good to me! 👍

@muan muan mentioned this pull request Jan 27, 2016
7 tasks
@jasonrudolph
Copy link
Collaborator Author

This is really close now. I see one test failure:

There are 246 emojis in "flags", but the expected number is 247

Off by one. Oh no. 🙀

@muan: Can you share how you got the counts that were introduced in 3a48994? That might help me track down the missing emoji.

@jasonrudolph
Copy link
Collaborator Author

@muan: Can you share how you got the counts that were introduced in 3a48994? That might help me track down the missing emoji.

Never mind. I see that there are indeed 247 flags:

screen shot 2016-01-27 at 7 49 07 am

Now I just have to track down the one that I'm missing. 🔍

@muan
Copy link
Owner

muan commented Jan 27, 2016

👋 Just saw that Slack prepends all the flag emojis with flag-, what do you think about doing that here as well?

image

Also, looks like the EU flag is missing 😁 🇪🇺🇪🇺🇪🇺

@jasonrudolph
Copy link
Collaborator Author

looks like the EU flag is missing 😁 🇪🇺🇪🇺🇪🇺

Thanks for catching that, @muan! 6bd97d0 adds the EU flag. And with that, we now have all 247 flags in place. 💃

Just saw that Slack prepends all the flag emojis with flag-, what do you think about doing that here as well?

Would you want to change the existing flag emojis to use that prefix as well? For example, would you want to change :es: to :flag-es:? If so, that would mean that apps like http://emoji.muan.co/ and https://github.com/muan/mojibar would output :flag-es: instead of :es:, right? And if that's true, then those apps would be outputting emoji codes that are incompatible with the emoji aliases used by GitHub and other services that use https://github.com/github/gemoji. Is that a concern?

@muan
Copy link
Owner

muan commented Jan 28, 2016

And if that's true, then those apps would be outputting emoji codes that are incompatible with the emoji aliases used by GitHub and other services that use https://github.com/github/gemoji. Is that a concern?

Ah you're totally right, let's keep it as is.

So this is done then? 🌟

@muan
Copy link
Owner

muan commented Jan 28, 2016

Also don't worry about the conflicts I can resolve them when merging. 👍

@jasonrudolph
Copy link
Collaborator Author

So this is done then?

Indeed! 😸

@muan muan merged commit 6bd97d0 into muan:master Jan 28, 2016
@muan
Copy link
Owner

muan commented Jan 28, 2016

⭐ ⭐ ⭐ ⭐ ⭐ ⭐ ⭐ ⭐ ⭐ ⭐
Thank you so much! 💥

@jasonrudolph jasonrudolph deleted the countries branch January 30, 2017 02:11
This pull request was closed.
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.

2 participants