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

[$1000] Add missing skin tones for handshake emoji #14302

Closed
6 tasks done
stitesExpensify opened this issue Jan 13, 2023 · 28 comments
Closed
6 tasks done

[$1000] Add missing skin tones for handshake emoji #14302

stitesExpensify opened this issue Jan 13, 2023 · 28 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review

Comments

@stitesExpensify
Copy link
Contributor

stitesExpensify commented Jan 13, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open emoji picker
  2. Scroll to handshake (or search for it)
  3. Change your skin tone to the darkest shade
  4. See that the emoji stays yellow

Expected Result:

The emoji skin tone is the shade you picked

Actual Result:

The emoji is yellow

Workaround:

No

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.2.51
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): All
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
image

Expensify/Expensify Issue URL: n/a
Issue reported by: @abdulrahuman5196
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1671716760354049?thread_ts=1671716674.530379&cid=C049HHMV9SM

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01523d2c03834a76c9
  • Upwork Job ID: 1614012452453519360
  • Last Price Increase: 2023-01-13
@stitesExpensify stitesExpensify added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 13, 2023
@stitesExpensify stitesExpensify self-assigned this Jan 13, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 13, 2023
@stitesExpensify stitesExpensify added External Added to denote the issue can be worked on by a contributor Engineering Improvement Item broken or needs improvement. labels Jan 13, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 13, 2023
@melvin-bot melvin-bot bot changed the title Add missing skin tones for handshake emoji [$1000] Add missing skin tones for handshake emoji Jan 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 13, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01523d2c03834a76c9

@melvin-bot
Copy link

melvin-bot bot commented Jan 13, 2023

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jan 13, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 13, 2023

Current assignee @stitesExpensify is eligible for the External assigner, not assigning anyone new.

@stitesExpensify
Copy link
Contributor Author

cc @abdulrahuman5196

@stitesExpensify
Copy link
Contributor Author

Coming from this post #13818 (comment)

If you can make another proposal here for posterity that would be great

@esh-g
Copy link
Contributor

esh-g commented Jan 13, 2023

Proposal

Solution:

diff --git a/assets/emojis.js b/assets/emojis.js
index 9a5a2f289..15e8747b0 100644
--- a/assets/emojis.js
+++ b/assets/emojis.js
@@ -2259,6 +2259,10 @@ const emojis = [
             'meeting',
             'shake',
         ],
+        types: [
+            "🤝🏾",
+            "🤝🏿",
+        ]
     },
     {
         name: 'pray',

A simple solution. I have only added a couple of types, but I can add how many types you want. Just give me a number

@stitesExpensify stitesExpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 13, 2023
@stitesExpensify
Copy link
Contributor Author

Hi @esh-g thank you for your proposal, however I created this issue specifically for @abdulrahuman5196 because they initially reported the bug, and have already presented a solution in a different issue. This is only open for organizational purposes.

@esh-g
Copy link
Contributor

esh-g commented Jan 13, 2023

Well that was unfortunate I guess. But anyway, congratulations @abdulrahuman5196

@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Jan 13, 2023

Thank you @stitesExpensify for opening the issue.
Providing the proposal from #13818 (comment) to add the shades for handshake icon. Kindly let me know if more information is required.

--- a/assets/emojis.js
+++ b/assets/emojis.js
@@ -2259,6 +2259,13 @@ const emojis = [
             'meeting',
             'shake',
         ],
+        types: [
+            '🤝🏿',
+            '🤝🏾',
+            '🤝🏽',
+            '🤝🏼',
+            '🤝🏻',
+        ],
     },

@abdulrahuman5196
Copy link
Contributor

@stitesExpensify Created a PR for the fix - #14308

@mananjadhav
Copy link
Collaborator

@abdulrahuman5196 Before I can review the PR can you please update the checklist properly for the PR.

@stitesExpensify @MitchExpensify Question, why is the scope limited to handshake? why not include other composite emojis like the following?

image

@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Jan 14, 2023

@mananjadhav I have updated the PR content link, and added a question in the PR. Kindly have a look into it.

Question, why is the scope limited to handshake? why not include other composite emojis like the following?

It seems currently the unicodes are not available for family composites - Discussed the same on the original issue -#13818

For reference, Even slack doesn't have shades for family emojis.

@mananjadhav
Copy link
Collaborator

Thanks for the update @abdulrahuman5196. Good to go, I've reviewed the PR. @stitesExpensify @nkuoch All yours to merge.

@MitchExpensify
Copy link
Contributor

Question, why is the scope limited to handshake? why not include other composite emojis like the following?

Good question. It seems like the default skin tone is not expected to be applied to the "family" group of emojis similarly to how the emoji facial expressions don't change based on default skin tone. Whereas all the hand emojis apart from 🤝 change so that feels like a bug.

@melvin-bot
Copy link

melvin-bot bot commented Jan 16, 2023

📣 @abdulrahuman5196 You have been assigned to this job by @stitesExpensify!
Please apply to this job in Upwork 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 📖

@MitchExpensify
Copy link
Contributor

Invites sent to @abdulrahuman5196 & @mananjadhav

Reminder to myself to pay the reporting bonus as a bonus on @abdulrahuman5196 Upwork contact

@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Jan 17, 2023

@MitchExpensify Thank you for the offer. I would also be eligible for 50% bonus, right? The PR was merged within 3 business days.

@MitchExpensify
Copy link
Contributor

I would also be eligible for 50% bonus, right?

Yes! I'll include that when paying

@abdulrahuman5196
Copy link
Contributor

@MitchExpensify Thank you. I had already accepted the offer 🤩

@mananjadhav
Copy link
Collaborator

@MitchExpensify @stitesExpensify This one is ready for payout as it was deployed on Jan 18, 2023. Can you please help me with the same? Please note, this is also eligible for the timeline bonus.

@MitchExpensify
Copy link
Contributor

On it! thanks @mananjadhav

I wonder why the issue title wasn't automatically updated with payment due date?

@MitchExpensify
Copy link
Contributor

All paid out with necessary bonuses! Thanks everyone

@mananjadhav
Copy link
Collaborator

@MitchExpensify I received the payout but it doesn't have the timeline bonus.

@MitchExpensify
Copy link
Contributor

Sorry about that! Paying now

@MitchExpensify
Copy link
Contributor

Done @mananjadhav

@mananjadhav
Copy link
Collaborator

Received the bonus. Thanks for the prompt action!

@abdulrahuman5196
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

5 participants