Closed m-natarajan closed 1 month ago
melvin-bot[bot]
commented
4 months ago Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
nkdengineer
commented
4 months ago Emoji is sent as syntax in step 2 Only :: shows in step 4
We are missing 🧌 icon in the list of emojis in here
We add all the missing icons to the list here
NA
Krishna2323
commented
4 months ago Unable to add Apple's standard emojis
The emoji is missing in the emoji list and translations file. https://github.com/Expensify/App/blob/525ad6ffede2f0127c0759855f020e54fc704403/assets/emojis/en.ts#L4 https://github.com/Expensify/App/blob/525ad6ffede2f0127c0759855f020e54fc704403/assets/emojis/es.ts#L4
We need to add the emoji in all three files.
In common.ts:
{
name: 'troll',
code: '🧌',
},In es.ts:
//
'🧌': {
keywords: ['troll', 'fairy tale', 'fantasy', 'monster'],
},In es.ts
'🧌': {
name: 'trol',
keywords: ['cara', 'cuento', 'fantasía', 'monstruo', 'trol'],
},Note: We might want to adjust the keywords, troll, fairy tale, fantasy, monster are the ones which is also used by slack. We might want to add more keywords like creature. I got these keywords from https://emojiterra.com/troll/. We will also check for other missing emojis.
melvin-bot[bot]
commented
4 months ago @kevinksullivan Whoops! This issue is 2 days overdue. Let's get this updated quick!
melvin-bot[bot]
commented
4 months ago @kevinksullivan Eep! 4 days overdue now. Issues have feelings too...
melvin-bot[bot]
commented
4 months ago @kevinksullivan 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
melvin-bot[bot]
commented
4 months ago @kevinksullivan 10 days overdue. I'm getting more depressed than Marvin.
kevinksullivan
commented
4 months ago Low priority VSB bug.
melvin-bot[bot]
commented
4 months ago Job added to Upwork: https://www.upwork.com/jobs/~01e356e9d1262d3053
melvin-bot[bot]
commented
4 months ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh (External)
Krishna2323
commented
4 months ago @rafecolton, I found that we are missing 80 emoji from versions 14.0, 15.0, and 15.1. We have all emojis from version 13.0 and earlier.
melvin-bot[bot]
commented
4 months ago @kevinksullivan @fedirjh this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
kevinksullivan
commented
3 months ago Not overdue, looking for proposals.
mananjadhav
commented
3 months ago Taking this over (thread).
mananjadhav
commented
3 months ago I think @Krishna2323 proposal makes sense. We should focus on adding all the missing emojis as a part of the scope.
In the long term we should improve on keeping these in sync. But I feel in the current issue we should only look at adding them manually.
🎀 👀 🎀 C+ reviewed.
melvin-bot[bot]
commented
3 months ago Triggered auto assignment to @thienlnam, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
rafecolton
commented
3 months ago I got this one :)
rafecolton
commented
3 months ago @kevinksullivan that proposal looks good to me, I agree with backfilling other emoji as part of this change as well.
@Krishna2323 I like your original suggestions to add the relevant keywords, so let's do that too
mananjadhav
commented
3 months ago Quick note @Krishna2323 @rafecolton, we'll have to take care of the skin tones too wherever applicable.
Krishna2323
commented
3 months ago @mananjadhav, thanks for the reminder, I have already included skin tones here https://github.com/Expensify/App/issues/42588#issuecomment-2148613429
melvin-bot[bot]
commented
3 months ago 📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
Krishna2323
commented
3 months ago @rafecolton @kevinksullivan, could we please increase the bounty for this issue? It is taking too much time and effort. Initially, the issue was meant to add a single missing emoji, but now we have to add 80+ emojis to the app.
I have already spent a lot of time finding the missing emojis before the PR, and now in the PR, I have to repeat these steps for each emoji:
Honestly, I think $1000 would be a fair amount, but I leave the final decision to you. Thanks
Krishna2323
commented
3 months ago PR Update: 39 out of 80 missing emojis have been added.
rafecolton
commented
3 months ago Thank you for the update! We will discuss the price internally and get back to you. When it comes time to assign an Expensify engineer to review the PR, please tag me and I will review it. (I am not currently in the group that gets auto-assigned, so by default it will pick somebody new.)
rafecolton
commented
3 months ago @Krishna2323 we discussed internally and agreed that $500 is a fair price for the change. I updated the issue title to reflect that, and @kevinksullivan will ensure you get paid the correct amount once this is deployed.
melvin-bot[bot]
commented
3 months ago Upwork job price has been updated to $500
rafecolton
commented
3 months ago Cool to see we automated that part 😄
mallenexpensify
commented
3 months ago @Krishna2323 , can we focus on the original scope of this issue, where we add the missing emoji for $500? Then, once the PR has hit production, we can discuss potential future improvements? If we decide to proceed there, you'd get first dibs on the job.
Krishna2323
commented
3 months ago @mallenexpensify, I suggest tackling all emoji-related issues at once. Implementing the changes as proposed here will eliminate the need to add emojis manually, as I'm doing in the PR for this issue. We can either close this issue or update the title, and I will update the PR to implement the changes mentioned in the Slack thread.
mallenexpensify
commented
3 months ago missing emoji
@Krishna2323, for that I mean all the emoji that are missing (apparently the plural of emoji is... emoji).
I will create a function that will do everything at once and return the expected emoji list.
Wonderful!
In the future, if any version releases, we just need to use that function and pass the latest emojis list from EmojiBase, then simply copy and paste to update our emoji list.
This is the part I don't want to get too caught up in. Can we consider a nice-to-have to maybe use in the future? If so, I'd be OK with trying to allow you dibs to work on the next emoji update. (my only concern is that it might be a while til there's another update, which means it might be tough to ensure you're tagged and offered the update to test/use your formula).
Either way... thanks for taking this on and for investing the time.
Krishna2323
commented
3 months ago This is the part I don't want to get too caught up in. Can we consider a nice-to-have to maybe use in the future? If so, I'd be OK with trying to allow you dibs to work on the next emoji update. (my only concern is that it might be a while til there's another update, which means it might be tough to ensure you're tagged and offered the update to test/use your formula).
@mallenexpensify, sorry but I didn't quite understand. Are you suggesting that we should only focus on adding missing emojis? If that's what you mean, then I can continue adding them manually like I was doing in that PR. However, my concern is different. If we only manually add those emojis, we'll miss addressing many issues mentioned here, which will eventually come up one by one. Additionally, the emoji list order is very messed up; you can compare it with Slack and the device emoji picker.
I have already prepared everything required to automate the process to update the emojis in the future in 1-2 very simple steps, but if you think we should only add missing emojis, then I'm fine with that as well. Sorry if I misunderstood something 😅
mananjadhav
commented
2 months ago I agree with @mallenexpensify here. @Krishna2323 Adding these emoji automation is good to have. Understand the scenario that adding manually for the ones we already know missing (~80) has crossed 3 weeks. If we go for an automation (which is a value-add) can take equally more amount of time to test thoroughly and ensure it's working.
Sorry I couldn't respond to your DM, but when I asked for formal proposal I meant add some beef to the technical details. Which again I am fine if you add that when we do prioritize the automation issue. I still feel the solution proposed in the slack thread needs a decent amount of grooming. (Personally I don't mind you using that automation to add these 80 emoji :P but I think we should look for closing this issue, as it's already more than a few weeks).
And about the emoji issues that you linked, @mallenexpensify we could just put them on Hold and if we see several instances we can work on it.
Krishna2323
commented
2 months ago @mananjadhav, will provide a more detailed proposal and a test branch today.
mananjadhav
commented
2 months ago @Krishna2323 What's the ETA on the PR for this issue?
mananjadhav
commented
2 months ago Quick bump @Krishna2323
Krishna2323
commented
2 months ago @mananjadhav, the test branch is in progress. Will ping you in few hours after completing it.
Krishna2323
commented
2 months ago There was no proper approach followed when adding/updating emojis.
emojibase-data/en/data.json, we have to separate emojis into groups. Each emoji has group property which will be used.Create a function which will take the emojis data obtained from emojibase and returns and object which will contain separated sorted emojis, for sorting we have order property on each emoji. The object returned will be like:
{
smileysAndEmotion: [...sortedEmojiList],
animalsAndNature: [...sortedEmojiList],
...
}function separetEmojiByGroups(emojis: EmojiBaseEmoji[]): Record<string, Emoji[]> {
const emojisByGroups: Record<string, Emoji[]> = {};
emojis.forEach((emoji) => {
if (emoji.group === undefined || emoji.version > 15) {
return;
}
if (emojisNamesByGroupIndex[emoji.group] && emojisByGroups[emojisNamesByGroupIndex[emoji.group]]) {
const filteredEmojiTypes = emoji.skins && emoji.skins?.length > 5 ? emoji.skins.filter((type) => type.hexcode.split('-').length <= 2) : emoji.skins;
emojisByGroups[emojisNamesByGroupIndex[`${emoji.group}`]].push({
name: emoji.label.split(' ').join('_'),
code: `${emoji.emoji}`,
keywords: emoji.tags,
types: filteredEmojiTypes?.sort((a, b) => (b.order ?? 0) - (a.order ?? -1)).map((type) => `${type.emoji}`),
order: emoji.order,
});
return;
}
if (emojisNamesByGroupIndex[emoji.group]) {
const filteredEmojiTypes = emoji.skins && emoji.skins?.length > 5 ? emoji.skins.filter((type) => type.hexcode.split('-').length <= 2) : emoji.skins;
emojisByGroups[emojisNamesByGroupIndex[emoji.group]] = [
{
name: emoji.label.split(' ').join('_'),
code: `${emoji.emoji}`,
keywords: emoji.tags,
types: filteredEmojiTypes?.sort((a, b) => (b.order ?? 0) - (a.order ?? -1)).map((type) => `${type.emoji}`),
order: emoji.order,
},
];
}
});
Object.keys(emojisByGroups).forEach((category) => {
// Sort the emojis in each category by the `order` property
emojisByGroups[category] = emojisByGroups[category].sort((a, b) => {
return (a.order ?? 0) - (b.order ?? 0);
});
});
return emojisByGroups;
}separetEmojiByGroups for obtaining grouped emojis for emojibase-data/en/data.json & emojibase-data/es/data.json data.emojis/sortedEmojisEN.ts, sortedEmojisES.ts)common.ts, en.ts, es.ts use the sorted data files to return the object according to the need.
import sortedEmojis from './sortedEmojisES';
import type {EmojisList} from './types';/ eslint-disable @typescript-eslint/naming-convention / const esEmojis: EmojisList = {};
Object.values(sortedEmojis) .map((e) => e) .forEach((val) => val.forEach((e) => (esEmojis[e.code] = {keywords: e.keywords ?? [], name: e.name})));
export default esEmojis;
This is the approach we will follow. Once this is implemented we would just need to call a function in `getUpdatedEmojis` (this will contain separetEmojiByGroups and data from emojibase) and it will return prepared object for `emojis/sortedEmojisEN.ts`, `sortedEmojisES.ts`. We just need to copy paste the object in `emojis/sortedEmojisEN.ts`, `sortedEmojisES.ts`.
### What alternative solutions did you explore? (Optional)@mananjadhav, pls check the proposal above, I would suggest to check the test branch since the changes are bit complex. The files to look into are getUpdatedEmojis.ts, commonNew.ts.
mananjadhav
commented
2 months ago @Krishna2323 I am a little confused, didn't we decide to just add the emojis here? cc - @mallenexpensify did we change the stance?
Krishna2323
commented
2 months ago @Krishna2323 I am a little confused, didn't we decide to just add the emojis here?
I was also 😅, should we complete this separately? That's what I was trying to ask here, if we want to complete this separately then I can complete this in a hour but what I was trying to say is that if we decide to go with this solution, we don't need to add these emojis separately. @mallenexpensify can you pls confirm 🙏🏻
mallenexpensify
commented
2 months ago Without knowing some of the particulars of the code and proposal, let's approach this as a v1 to get the emojis added then we can improve/streamline things after.
mananjadhav
commented
2 months ago @Krishna2323 Still waiting for an update on this one.
Krishna2323
commented
2 months ago Without knowing some of the particulars of the code and proposal, let's approach this as a v1 to get the emojis added then we can improve/streamline things after.
Sorry I missed this comment, I will complete the PR today.
Krishna2323
commented
2 months ago @mananjadhav, PR ready for review.
melvin-bot[bot]
commented
2 months ago Triggered auto assignment to @deetergp, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
deetergp
commented
2 months ago @Krishna2323 @mananjadhav is it possible that we caused this blocker?
melvin-bot[bot]
commented
2 months ago ⚠️ Looks like this issue was linked to a Deploy Blocker here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
melvin-bot[bot]
commented
2 months ago Reviewing label has been removed, please complete the "BugZero Checklist".
melvin-bot[bot]
commented
2 months ago The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.14-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-08-07. :confetti_ball:
For reference, here are some details about the assignees on this issue:
melvin-bot[bot]
commented
2 months ago BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.75-0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @rafecolton Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1716485230886559
Action Performed:
Expected Result:
Describe what you think should've happened Emoji is sent in the step 2 When hovering the emoji should show the added emoji in step 4
Actual Result:
Emoji is sent as syntax in step 2 Only
::shows in step 4Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/38435837/c78406fa-232e-4c5a-80e4-974eb6c99fff
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @kevinksullivan