-
Notifications
You must be signed in to change notification settings - Fork 76
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 emojis + update emoji keywords + different emoji categories + updated pot + update it language + new icons #57
base: master
Are you sure you want to change the base?
Conversation
What is not easier is to understand what actually changed in the file by reading the diff... Readability is more important than commenting with On a similar topic, changing some spaces/tabs in extension.js wasn't useful. I'll merge it but i'll probably change a few things
I don't have 3.32 files on my computer, could you add them all ? for example the actual food&drink icon instead of this thing i stole from the Caffeine extension |
I basically re-wrote emojisCharacters.js and emojisKeywords.js again. I haven't just added/removed emojis/keywords. The
I thought it was easier this way: you can open both files and, if your text editor shows the line number, you can see that at the same line number you have the icon in one file and its keywords in the other one.
Me neither. I only took the missing icons from the current gtk master branch and put the in 'icons' folder. I can add all of them, if you want. I'm sorry for messing up things... Well... |
Yes, and that's the issue: i can't see what has been added/removed, because almost every line in the file has changed. What have you added ? (new emojis ? which ones ?) What have you removed ? (and why ?)
I use a gedit plugin named "spaces indicators" for that |
At some point I noticed that some emoticon was missing, like some grinning face. It would've taken a lot of time to check and add only those missing icons. It's still the same extension, and those two files still follow the same scheme you gave them, with the only exception that you have 1 emoji per line instead of 10. This, as I said, because I wanted to have the same line number for both the icon and its keywords: this should simplify any future adding of new keywords. You seem so grumpy. I'm not forcing you to merge anything. I wanted to updated the list of emojis and propose what I consider a better categorization. |
I am lol, sorry, but it's not because of you, i really appreciate your work, but i've a ton of exams and no time to carefully review what has changed. I'll merge it, but probably during week 52: it will require time because i need to keep the current keywords, which as you can see contain both:
That's why i would have prefer to see exactly what has changed, so i could have keep the new categories and emoji list, but with the current keywords list that i would have completed by comparing what emojis were added |
Understood. |
Hey man, |
i love the idea of "text smileys" as keywords, does it already work with the current code ? |
Yes. :) What's to you think about sharing that spreadsheet and make it available to users so that they can add their suggested keywords? |
Yes, i could add a link in the readme for example |
Let me complete that spreadsheet and then I'll go back to you. :) |
Done! You should have the same permissions I have to edit everything. --- x ---
Thank you, maestro. :) |
These redundant words were on the list from the unicode website, i keep them for translations (official translated names might not include reference to "face" or "eyes" even if it's pertinent) |
🤔 |
the important word here is "if" 😄 |
Ok. Got it. :) Hey! |
Yes, merry Christmas 🎅 |
}, | ||
|
||
_addAllCategories: function() { | ||
for (let i = 0; i< 9; i++) { | ||
for (let i = 0; i< 11; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe < this.emojiCategories.length
(or even a forEach
, for this case) would be more appropriate here (so in the future people don't need to worry about modifying this. The same goes for line 758 below.
I'm new to the project though, I don't know if there is any limitation to do this.
Hi!
I updated all the things you can read in the title.
It's a lot of stuff. You might not appreciate all of them.
Just take a look and see what can be merged or rejected. :)