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

[HOLD for payment 2021-09-22] Size of the single emoji in the chat is too small #4543

Closed
mananjadhav opened this issue Aug 11, 2021 · 53 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@mananjadhav
Copy link
Collaborator

mananjadhav commented Aug 11, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Upwork Link: https://www.upwork.com/jobs/~01d853d1f00586e2de


With respect to comment #4456 (comment), adding this issue.

Action Performed:

  1. Go to chat, click on EmojiPicker
  2. Enter some Emojis (one emoji at a time), especially people & faces

Expected Result:

Emoji should be a little bigger
From ExpensifyChat,
image

From Slack,
image

and honestly, we should also increase inline emojis by at least 2 points. (debatable)

Actual Result:

image
image

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

@mananjadhav mananjadhav added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Aug 11, 2021
@MelvinBot
Copy link

Triggered auto assignment to @MitchExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Aug 11, 2021
@mananjadhav mananjadhav changed the title Size of the Emoji in the chat is too small Size of the single emoji in the chat is too small Aug 11, 2021
@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Aug 11, 2021

Proposal

At the moment ValidationUtils, we're trying to match if it is isSingleEmoji, but the message.length and matchedMessage.length is different. This could be because of the Unicode characters in the emojis.

<Text
      selectable={!canUseTouchScreen() || !this.props.isSmallScreenWidth}
      style={isSingleEmoji(fragment.text) ? styles.singleEmojiText : (isEmoji())}
>

There are two approaches to solve this:

In ValidationUtils, we should update the isSingleEmoji function to the following:

Convert Emoji to Uncide and then compare. (recommended)

function isSingleEmoji(message) {
    const match = message.match(CONST.REGEX.EMOJIS);

    if (!match) {
        return false;
    }

    const matchedEmoji = match[0];
    console.log('Emoji Text', emojiUnicode(message) === emojiUnicode(matchedEmoji));
    return emojiUnicode(message) === emojiUnicode(matchedEmoji);
}

function emojiUnicode(emoji) {
    let comp;
    if (emoji.length === 1) {
        comp = emoji.charCodeAt(0);
    }
    comp = (
        (emoji.charCodeAt(0) - 0xD800) * 0x400
      + (emoji.charCodeAt(1) - 0xDC00) + 0x10000
    );
    if (comp < 0) {
        comp = emoji.charCodeAt(0);
    }
    return comp.toString('16');
}

or somehow trim or add +1 to the message. (not recommended)

function isSingleEmoji(message) {
    const match = message.match(CONST.REGEX.EMOJIS);
    if (!match) {
        return false;
    }
    const matchedEmoji = match[0];
    return (matchedEmoji.length + 1) === message.length; 

}

@parasharrajat
Copy link
Member

#4114 Sounds familiar so that can be merged into this one.

@rdjuric
Copy link
Contributor

rdjuric commented Aug 11, 2021

@parasharrajat I think that PR is about also making the emojis bigger in other scenarios - like on the Composer and with multiple emojis in the same message.

This one looks like a regression from my #4206 PR. I added a VS unicode to our emojis but forgot to update the isSingleEmoji validation. I can open a PR to fix this 😅

@parasharrajat
Copy link
Member

parasharrajat commented Aug 11, 2021

Sure @rdjuric. I was wondering that you implemented this feature earlier and how is it not working?
But I didn't mention regression to keep few contributors happy. Alas, skipping long discussion.

@mananjadhav
Copy link
Collaborator Author

Thanks for notifying. @rdjuric You'll pick this and close it?

@MitchExpensify
Copy link
Contributor

Unique - There are similar issues here #4422 and here #4544
Issue hygiene - Good
Clarity of problem - Not expressed as a problem but rather a comparison with Slack. In other words, what is the problem with the current size of emojis in Expensify Chat? Are they too small to identify details for example? (I agree t is hard to see detail which takes away from them) Are there specific size inconsistencies? (I notice inconsistencies as mentioned here) by @mallenexpensify
Value - I do think its valuable to easily be able to see emojis in more detail and making them bigger is a perfectly reasonable solution to that.

If we're going to cover emoji size with this issue, we should close this right @mallenexpensify ?

@mananjadhav
Copy link
Collaborator Author

#4422 is similar to the current one, which deals with the size and rendering of emojis in the chats, conditional render based on 1 or more emojis.

#4544 is more of a UX item for Emoji Picker. IMO, not related.

@parasharrajat
Copy link
Member

Anyone looking into this. This is a regression and looks like Rujuric is working over it #4543 (comment).

@jboniface
Copy link

Does anyone know what PR caused the regression?

@parasharrajat
Copy link
Member

parasharrajat commented Aug 13, 2021

#4206 Regression from...

@jboniface
Copy link

oh sorry, i missed that in the comment somehow. still opening my eyes this morning.

@MelvinBot
Copy link

@MitchExpensify Eep! 4 days overdue now. Issues have feelings too...

@MitchExpensify
Copy link
Contributor

Labelling external seeing as we'e consolidating to this issue.

@MelvinBot MelvinBot removed the Overdue label Aug 17, 2021
@MitchExpensify MitchExpensify added the External Added to denote the issue can be worked on by a contributor label Aug 17, 2021
@MitchExpensify MitchExpensify removed their assignment Aug 17, 2021
@MitchExpensify MitchExpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Aug 17, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Jag96 (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@mananjadhav
Copy link
Collaborator Author

@mananjadhav Sounds good! It's great to know that you have some experience with emoji-regex.

I've used emoji-regex package earlier and it isn't full proof. It doesn't match many emojis such as shopping bag, chilly, mostly sunny, etc.

Well, we can always submit a pull request to improve it! Overall I'm usually a big fan of contributing to community packages to help keep our own repos a bit lighter and to benefit from community maintenance.

+1.

But in this case maybe it's not worth it. I wonder if those emoji are missing from their test suite for some reason?

As you rightly said might not be worth for this one. We could pick the regex and improve it? The regex itself is 13kb but the overall unpackaged size is 97kb. I am not sure of the reason these are missing from their test suite.

Which makes me think ... if we are going to maintain the emoji regex ourselves we should absolutely add some unit tests for it.

Yeah we should do that. My suggestion would be:

  1. To pick all the emojis in Emoji.js and ensure that our regex matches each of them.
  2. Some sample chats with Emojis in between and our function should pick these.

@mananjadhav
Copy link
Collaborator Author

@deetergp @roryabraham Should I then start with the PR?

@deetergp
Copy link
Contributor

Yes, @mananjadhav, go for it 👍

@mallenexpensify
Copy link
Contributor

@deetergp 'go for it' means @Jag96 should hire in Upwork, right?

@Jag96
Copy link
Contributor

Jag96 commented Aug 31, 2021

@mananjadhav I've invited you to the Upwork job

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 31, 2021
@Jag96 Jag96 added Weekly KSv2 and removed Daily KSv2 labels Aug 31, 2021
@mananjadhav
Copy link
Collaborator Author

Thanks @Jag96

@deetergp
Copy link
Contributor

This one is in progress and pending review from @marcaaron, not overdue.

@MelvinBot MelvinBot removed the Overdue label Sep 10, 2021
@deetergp deetergp added the Reviewing Has a PR in review label Sep 10, 2021
@botify
Copy link

botify commented Sep 13, 2021

🚀 Deployed to staging by @marcaaron in version: 1.0.97-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Sep 15, 2021
@botify botify changed the title Size of the single emoji in the chat is too small [HOLD for payment 2021-09-22] Size of the single emoji in the chat is too small Sep 15, 2021
@botify
Copy link

botify commented Sep 15, 2021

🚀 Deployed to production by @francoisl in version: 1.0.98-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@botify botify added Weekly KSv2 and removed Weekly KSv2 labels Sep 20, 2021
@botify botify changed the title [HOLD for payment 2021-09-22] Size of the single emoji in the chat is too small [HOLD for payment 2021-09-27] [HOLD for payment 2021-09-22] Size of the single emoji in the chat is too small Sep 20, 2021
@botify
Copy link

botify commented Sep 20, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.0.98-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2021-09-27. 🎊

@mountiny mountiny changed the title [HOLD for payment 2021-09-27] [HOLD for payment 2021-09-22] Size of the single emoji in the chat is too small [HOLD for payment 2021-09-22] Size of the single emoji in the chat is too small Sep 20, 2021
@mountiny
Copy link
Contributor

This message is a bug, caused by today's deploy and some weird GH API behaviour. Updated the title back to what it was 🙇

@mananjadhav
Copy link
Collaborator Author

@deetergp @Jag96 Are the payments also on hold due to n6?

@Jag96
Copy link
Contributor

Jag96 commented Sep 27, 2021

Thanks for the reminder @mananjadhav, paid + bonus!

@Jag96 Jag96 closed this as completed Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests