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

[$500] MEDIUM: Mouse over emoji to get name #34307

Closed
quinthar opened this issue Jan 11, 2024 · 55 comments
Closed

[$500] MEDIUM: Mouse over emoji to get name #34307

quinthar opened this issue Jan 11, 2024 · 55 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@quinthar
Copy link
Contributor

quinthar commented Jan 11, 2024

Strategy:
Making the workplace fun is good business. One way to do that is by supporting emoji, to make conversations more fun.

Problem:
Many emoji are kind of unclear, meaning the joke falls flat or whatever message you try to send might be missed.

Solution:
On desktop, when you hover over an emoji, show the name of that emoji. Pay particular attention to performance, as previous hover states have sometimes introduced major performance regressions -- ask in #expensify-open-source for advice.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bf03f6fb12beb641
  • Upwork Job ID: 1745259225093054464
  • Last Price Increase: 2024-01-18
  • Automatic offers:
    • fedirjh | Reviewer | 28118473
    • dukenv0307 | Contributor | 28118474
@quinthar quinthar converted this from a draft issue Jan 11, 2024
@quinthar quinthar added External Added to denote the issue can be worked on by a contributor Daily KSv2 NewFeature Something to build that is a new item. labels Jan 11, 2024
@melvin-bot melvin-bot bot changed the title MEDIUM: Mouse over emoji to get name [$500] MEDIUM: Mouse over emoji to get name Jan 11, 2024
Copy link

melvin-bot bot commented Jan 11, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01bf03f6fb12beb641

Copy link

melvin-bot bot commented Jan 11, 2024

@melvin-bot melvin-bot bot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors and removed Daily KSv2 labels Jan 11, 2024
Copy link

melvin-bot bot commented Jan 11, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jan 11, 2024
@dukenv0307
Copy link
Contributor

dukenv0307 commented Jan 11, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

We want to show emoji name on hover on emoji, I assume this only applies to Emojis that appear in the chat.

What is the root cause of that problem?

This is new feature

What changes do you think we should make in order to solve the problem?

(All below are pseudocode and we can polish that and fix minor cases in the PR if the solution is accepted)

  1. Add a rule in ExepnsifyMark that will wrap the emojis in the comment inside a <emoji></emoji> tag, based on the CONST.REG_EXP.EMOJIS Regex
  2. Add an EmojiRenderer that's responsible for rendering the emoji, and put it in here to handle the emoji tag
'emoji': EmojiRenderer,
  1. The EmojiRenderer will render the emoji inside a Tooltip, with renderTooltipContent that renders according to the design
  2. Address minor cases like if the emojis are the only thing in a comment (no other texts), show an enlarged emojis

Some notes:

  • The emoji name can be retrieved similar to here (we need to get the emoji by code and then get the name that way)
  • The containsEmoji here currently has a bug because it's using a global regex so the regex validation result will be cached accross checks, we need to construct a new RegExp based on CONST.REGEX.EMOJI pattern inside containsEmoji so it will have a new regex every time.
  • For platforms where tooltip is not available (like native), of course we'll return just pure Text inside EmojiRenderer so it doesn't affect those platforms in any way.

We can have custom design for the Tooltip, or use the default Tooltip style, based on design team's opinion.

What alternative solutions did you explore? (Optional)

I don't think we should apply the emoji tooltips for text inside TextInput because the user typing already knows the emoji name (need to know the name to search via autosuggestion or emoji picker), but if we do, we'll need additional handling.

@dragnoir
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Hovering eùojies does not display names.

What is the root cause of that problem?

Missing the hover action when hovering emojies.

What changes do you think we should make in order to solve the problem?

I suggest displaying the emoji name on this area:

image

Then it will be like that:

2024-01-11 09 38 57

We can add a function here to set hevered emoji

Change the skin tone icon to the right

<View style={[styles.flexRow, styles.p3, styles.ph4, styles.emojiPickerContainer]}>
{!isSkinToneListVisible && (
<PressableWithoutFeedback
onPress={toggleIsSkinToneListVisible}
style={[styles.flexRow, styles.alignSelfCenter, styles.justifyContentStart, styles.alignItemsCenter]}
accessibilityLabel={translate('emojiPicker.skinTonePickerLabel')}
role={CONST.ROLE.BUTTON}
>
<View style={[styles.emojiItem, styles.wAuto, styles.justifyContentCenter]}>
<Text style={[styles.emojiText, styles.ph2, styles.textNoWrap]}>{currentSkinTone.code}</Text>
</View>
<Text style={[styles.emojiSkinToneTitle]}>{translate('emojiPicker.skinTonePickerLabel')}</Text>
</PressableWithoutFeedback>

Add new component on the left for displaying the Emoj name.

@zanyrenney
Copy link
Contributor

@fedirjh please review the proposals above for this!

@fedirjh
Copy link
Contributor

fedirjh commented Jan 11, 2024

@zanyrenney What sould the tooltip for the emoji contains ? is it just text , or text with emoji , similar to what's slack doing , for reference :

Slack Just Text
Screenshot 2024-01-11 at 4 33 00 PM Screenshot 2024-01-11 at 4 34 18 PM

@fedirjh
Copy link
Contributor

fedirjh commented Jan 12, 2024

cc @stitesExpensify I would like to get your inputs on this issue.

@stitesExpensify stitesExpensify self-assigned this Jan 12, 2024
@melvin-bot melvin-bot bot added the Overdue label Jan 15, 2024
@zanyrenney
Copy link
Contributor

Great question, thanks for bringing this up @fedirjh. I'm going to take this to the channel and I will tag Brandon and David for their input seeing as they are the ones that have reported/worked on this in the past.

@melvin-bot melvin-bot bot removed the Overdue label Jan 15, 2024
@zanyrenney
Copy link
Contributor

@stitesExpensify
Copy link
Contributor

@fedirjh we decided to go with the slack style version with an enlarged version of the emoji

@fedirjh
Copy link
Contributor

fedirjh commented Jan 16, 2024

@dukenv0307 Could you please update your proposal to match the expected style in #34307 (comment) ?

Regarding the solution, considering our prior successful implementation of a similar use case involving user mentions, which required a custom renderer within the HTMLEngineProvider, I would like to propose adopting a similar approach for the implementation of an emoji renderer. Could we explore this solution?

@dukenv0307
Copy link
Contributor

@dukenv0307 Could you please update your proposal to match the expected style in #34307 (comment) ?

@fedirjh sure, we just need to pass custom renderTooltipContent to the Tooltip for the emoji.

Regarding the solution, considering our prior successful implementation of a similar use case involving user mentions, which required a custom renderer within the HTMLEngineProvider, I would like to propose adopting a similar approach for the implementation of an emoji renderer. Could we explore this solution?

@fedirjh sure, we can have a EmojiRenderer similar to MentionUserRenderer, I'll post the details for that soon

Copy link

melvin-bot bot commented Jan 18, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Copy link

melvin-bot bot commented Mar 6, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@fedirjh] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@zanyrenney] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Mar 13, 2024
@dukenv0307
Copy link
Contributor

@fedirjh Create another PR Expensify/expensify-common#664 here since the previous PR is reverted for the new bump expensify-common.

@roryabraham roryabraham changed the title [HOLD for payment 2024-03-13] [$500] MEDIUM: Mouse over emoji to get name [$500] MEDIUM: Mouse over emoji to get name Mar 13, 2024
@roryabraham roryabraham removed the Awaiting Payment Auto-added when associated PR is deployed to production label Mar 13, 2024
@roryabraham
Copy link
Contributor

removing the HOLD for payment since the first PR was reverted

@stitesExpensify
Copy link
Contributor

I believe we're just waiting for the PR to be live for a week and then we can issue payment here. We just deployed to prod 2 days ago

@melvin-bot melvin-bot bot removed the Overdue label Mar 28, 2024
@robertKozik
Copy link
Contributor

Hey hey, coming with a simple question. Is it intended to parse an emoji inside code blocks? We are getting an error on parsing in react-native-live-markdown due to adding html tags inside html attributes when emoji is present there and I'm wondering if I can simply prohibit parsing emoji within code blocks 🤔🤔🤔

@fedirjh
Copy link
Contributor

fedirjh commented Apr 2, 2024

Is it intended to parse an emoji inside code blocks?

@robertKozik Nope, it's a bug that will be fixed in #14676, check this #34307 (comment)

@dukenv0307
Copy link
Contributor

@zanyrenney The PR was deployed to production last week. Please add Awaiting payment label.

@melvin-bot melvin-bot bot added the Overdue label Apr 11, 2024
@stitesExpensify stitesExpensify added the Awaiting Payment Auto-added when associated PR is deployed to production label Apr 11, 2024
@stitesExpensify
Copy link
Contributor

Hm looks like the bot doesn't want to add the payment comments? Do we need the bug label instead of new feature maybe?

@melvin-bot melvin-bot bot removed the Overdue label Apr 11, 2024
@fedirjh
Copy link
Contributor

fedirjh commented Apr 12, 2024

I guess this needs a regression test:

Regression Test Proposal

1. Open App and select a chat.
2. Send a message containing emojis
3. Hover over the emoji and verify that a tooltip appears 
4. Verify tooltip displays an enlarged emoji and the emoji name
  • Do we agree 👍 or 👎

@zanyrenney
Copy link
Contributor

payment summary

paid @fedirjh $500 via upwork
paid @dukenv0307 $500 via upwork.

Closing!

@melvin-bot melvin-bot bot removed the Overdue label Apr 22, 2024
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 Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
No open projects
Development

No branches or pull requests

9 participants