feat: add/remove reaction #15210

hannojg · 2023-02-16T14:55:53Z

Details

Implementation for adding and removing reactions, according to:

[HOLD for payment 2023-03-17] Add new AddEmojiReaction and RemoveEmojiReaction #15125

Known issues:

When sending reactions very rapidly it can happen that a pusher event with stale information overwrites the local state, and thus causing buggy behaviour. This is a known issue with the current architecture. See this video for an example of the bug: https://user-images.githubusercontent.com/16821682/222969430-873513c0-63d5-4a84-95f5-72a13377762e.mov
When editing a message the reactions will get removed. Here pusher sends an event where the reactions property in the message object is missing. This needs to be fixed elsewhere I believe.

(Done) TODO:

8bc100b Fix issue where components don't receive update to re-render when onyx data changes
1cb6d60 Fix regression with mini context menu
55834f6 Fix bug where removing a reaction leaves an artefact
- https://user-images.githubusercontent.com/16821682/222957501-5941c188-4a35-4d05-83ad-7c06fc7c7176.mov
e9ba4f5 Fix bug:
- User A add reaction
- User B react as well
- User B try to remove reaction -> Doesn't work, instead of removing it tries to add the reaction again

Fixed Issues

$ #15125
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Quick reactions are working

Test 1: Web specific, hover menu:

Login with user A
Open a chat with user B
Send a message
Hover over the send message with the mouse
Expect the context menu to show in the top right corner, and it shows the four quick reaction emojis as well as a icon button to open the emoji picker
When pressing on the "👍" reaction, the "👍" gets added as reaction to the message
When pressing the "👍" in the menu again the reaction gets removed

Test 2: Quick reactions from context menu

Login with user A
Open a chat with user B
Send a message
When right pressing (on web/desktop), or long pressing (mobile) the context menu opens
When pressing on the "👍" reaction, the "👍" gets added as reaction to the message
The context menu closes after pressing the reaction
Open the context menu again and press the "👍" in the menu again -> the reaction gets removed

The emoji picker is working

Test 1: Web specific, hover menu:

Login with user A
Open a chat with user B
Send a message
Hover over the send message with the mouse
Expect the context menu to show in the top right corner, and it shows the four quick reaction emojis as well as a icon button to open the emoji picker
Press the icon button with the emoji to open the emoji picker
Expect the hover menu to stay open while the emoji picker opens next to the icon button
When selecting an emoji it gets added to the message as reaction
Upon selecting an emoji the emoji picker closes
When opening the emoji picker again and pressing the same emoji again it gets removed from the message

Test 2: Emoji picker from context menu

Login with user A
Open a chat with user B
Send a message
When right pressing (on web/desktop), or long pressing (mobile) the context menu opens
Press the icon button with the emoji to open the emoji picker
Expect the context menu to dismiss and instead the emoji picker shows
When selecting an emoji it gets added to the message as reaction
Upon selecting an emoji the emoji picker closes
When opening the emoji picker again and pressing the same emoji again it gets removed from the message

Test 3: Emoji picker from reactions

Login with user A
Open a chat with user B
Send a message
Add a reaction by either using the picker or quick reactions, doesn't matter
Expect that next to the reaction that now shows underneath the message a "add emoji" icon button appeared
When pressing it expect the emoji picker to open
When selecting an emoji it gets added to the message as reaction
Upon selecting an emoji the emoji picker closes
When opening the emoji picker again and pressing the same emoji again it gets removed from the message

📩 Sending & receiving works

Login with user A on device A
Login with user B on device B
User A sends a chat message to user B
User B reacts with a 👍 (👍) to the message of user A
Both user A and user B can see the reaction on the message user A sent
For user B the reaction underneath the message is highlighted (so it's obvious to user B that he/she added the reaction)
User A now reacts as well with a 👍
For both users the reaction counter increments to 2
Both users now remove their reaction
For both users the reaction is gone
User A now adds another emoji reaction, which isn’t the 👍 (e.g. 😊 😊)
After that User B now adds again a 👍 reaction
We confirm that for both users the 👍 reaction is after the 😊 reaction user A sent (chronological order)

Adding the same reaction twice with different skin tone

Login with user A
Open a chat with user B
Send a message
Add a "👍" as reaction
Now open the emoji picker for adding a reaction
Change the default skin color to something else than the yellow tone
Add the "👍" emoji again but this time with the different skin tone
Verify that the reaction bubble underneath the message now shows both emoji skin tones and the counter says "2". Also make sure that the emoji appear in the correct order (e.g. first the yellow one, then the one with the skin tone that you picked)

Offline tests

Login with user A on device A
Login with user B on device B
User A sends a chat message to user B
After user B receives the message
Put the device of user B offline
User B reacts with a 👍 to the message of user A
User B sees the reaction immediately, while user A doesn't yet
User B goes online again
User A receives the emoji reaction User B sent

QA Steps

See testing steps above

PR Author Checklist

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

Screen.Recording.2023-02-19.at.16.48.58.mov

Mobile Web - Safari

Simulator.Screen.Recording.-.iPhone.14.-.2023-02-19.at.16.45.56.mp4

Desktop

desktop.mov

iOS

Simulator.Screen.Recording.-.iPhone.14.-.2023-02-19.at.16.47.24.mp4

Android

Screen.Recording.2023-02-19.at.16.55.13.mov

…no/feat-add-remove-reactions

stitesExpensify

LGTM and tests well 🎉 LET'S DO THIS!

stitesExpensify · 2023-03-08T13:55:46Z

Waiting for verify_podfile....

OSBotify · 2023-03-08T14:00:52Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

0xmiros · 2023-03-08T14:06:06Z

Sorry, let's fix this quickly in new PR. Happens after log out

0xmiros · 2023-03-08T14:22:23Z

src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js

+                        // make sure that renderContent isn't mixed with unsupported props
+                        if (__DEV__ && (contextAction.text != null || contextAction.icon != null)) {
+                            throw new Error('Dev error: renderContent() and text/icon cannot be used together.');
+                        }


Also remove this dev code

Honestly I think this is a good warning to have, I wouldn't mind keeping it

0xmiros · 2023-03-08T14:26:26Z

While digging into above console warning, I found performance issue on main (before this PR).
BaseReportActionContextMenu component is rendered unnecessarily on app launch (even when logged out).
This happens because of this code:

App/src/Expensify.js

Lines 203 to 205 in f4ffc7c

    
           <PopoverReportActionContextMenu 
        
               ref={ReportActionContextMenu.contextMenuRef} 
        
           />

0xmiros · 2023-03-08T14:36:32Z

@stitesExpensify I think it would be good to announce in channel about known issues we discussed in this PR so that contributors or QA team won't do duplicated bug report which we are already aware of and planned to fix.

OSBotify · 2023-03-09T04:20:05Z

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.2.81-0 🚀

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

parasharrajat · 2023-03-10T11:04:51Z

src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js

+                            throw new Error('Dev error: renderContent() and text/icon cannot be used together.');
+                        }
+
+                        return contextAction.renderContent(closePopup, payload);


I will prefer the following pattern instead of renderContent if there are no hidden tricks.

If you want the user to provide UI instead of rendering the default UI via ContextMenuItem.

Then we can accept a functional Component instead of Object in ContextMenuActions.

It means that the first act of ContextMenuActions you added in this PR can simply be a functional component that will accept all the values as props.

This will also remove the possibility of user error where the user can provide text or icon and renderContent together.

OSBotify · 2023-03-10T14:06:26Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.2.81-1 🚀

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

parasharrajat · 2023-03-29T16:47:07Z

src/components/Reactions/EmojiReactionBubble.js

+        <Pressable
+            style={({hovered}) => [
+                styles.emojiReactionBubble,
+                StyleUtils.getEmojiReactionBubbleStyle(hovered, hasUserReacted, props.sizeScale),


Pressed state styles were missed here. #15813

puneetlath · 2023-04-03T20:12:31Z

Friendly FYI: this PR created a very small regression in the copy: #15867. Should've been Edit reaction instead of Edit Reaction in order to be consistent with how we do it everywhere else in the app.

stitesExpensify · 2023-04-05T16:25:01Z

Ach dangit. Thanks for letting me know

sobitneupane · 2023-04-12T07:03:10Z

This PR created a regression: #16384. On navigation to Add reaction bubble using keyboard, the emoji picker appears in top left corner.

sobitneupane · 2023-04-21T11:53:16Z

With this PR, we have updated the size of emoji in reaction of a message to 28px with a font size of 15px, in mini context menu to 20px with a font size of 13px and in context menu to 34px with a font size of 17px.

Those changes might have been missed or not considered in this PR.

parasharrajat · 2023-05-26T13:12:19Z

src/components/Reactions/AddReactionBubble.js

+                    hovered,
+                    pressed,
+                }) => [
+                    styles.emojiReactionBubble,


We should prevent selection on all buttons where long-press action is supported as it moves the selection around the UI on safari iOS.
#18661

hoangzinh · 2024-01-22T15:05:47Z

src/components/BaseMiniContextMenuItem.js

+            ref={props.innerRef}
+            focusable
+            onPress={props.onPress}
+            accessibilityLabel={props.tooltipText}


Coming from #33547, we should add prop role={CONST.ROLE.BUTTON} if the Pressable is a button/icon button to avoid text selection issue on double click.

rushatgabhane

Small bug fixed: Clicking on same emoji reaction changes skin tone instead of removing

hannojg added 26 commits February 15, 2023 12:34

wip: render quick reactions in context menu

80333cb

wip: adjustments for web

071b88f

add actions for adding and removing reactions

a984a0b

wip: add reaction when using quick reactions

f96b73b

render emoji reactions underneath a message

a164fa1

TEMP: REMOVE ME once accountID on personal details is live

f0926c1

change from login to accountID

318931f

wip: adding / removing

549bcbe

change: when removing reactions, remove the whole emoji object

c7de8cd

cleanup code

7281f82

show AddReactionBubble in the row of reactions as well

7907fd2

implemented toggling reactions when selecting the same again

71220c0

close context menu when open emoji picker or selected emoji

44909ab

fix emoji picker being wrong positioned

8042b9b

wip: mini menu

483b119

wip: mini menu adding add reaction icon

20d463c

fix: mini menu open picker wrong positioned + was closing

99bfb91

Merge branch 'main' of github.com:margelo/expensify-app-fork into han…

2f61782

…no/feat-add-remove-reactions

fix: open emoji picker on mobile

85357b1

clean up

4c07ba2

rename and document functions

e064297

pick the preferred skin tone

52a9135

updated JSDocs

c8c450c

updated JSDocs

bf7d78e

updated JSDocs

f874ea5

remove todos

267338e

hannojg marked this pull request as ready for review February 19, 2023 16:16

hannojg requested a review from a team as a code owner February 19, 2023 16:16

hannojg mentioned this pull request Feb 19, 2023

[HOLD for payment 2023-03-17] Add new AddEmojiReaction and RemoveEmojiReaction #15125

Closed

melvin-bot bot removed the request for review from a team February 19, 2023 16:17

hannojg dismissed 0xmiros’s stale review via 9e3596f March 8, 2023 13:49

stitesExpensify approved these changes Mar 8, 2023

View reviewed changes

stitesExpensify merged commit f4ffc7c into Expensify:main Mar 8, 2023

0xmiros reviewed Mar 8, 2023

View reviewed changes

perunt mentioned this pull request Mar 8, 2023

Reaction fixes #15750

Merged

55 tasks

OSBotify mentioned this pull request Mar 9, 2023

Deploy Checklist: New Expensify 2023-03-09 #15767

Closed

19 tasks

kbecciv mentioned this pull request Mar 9, 2023

Emoji Reaction - Some emojis are displayed as broken #15790

Closed

6 tasks

parasharrajat reviewed Mar 10, 2023

View reviewed changes

mananjadhav mentioned this pull request Mar 29, 2023

[HOLD for payment 2023-03-31] [$1000] Inconsistency in the tooltip texts between 'Edit comment/ Delete comment' & 'Add Reaction' #15867

Closed

6 tasks

parasharrajat reviewed Mar 29, 2023

View reviewed changes

parasharrajat mentioned this pull request Mar 29, 2023

[HOLD for payment 2023-03-23] [$1000] Emoji reaction is not highlighted when holding it down #15813

Closed

6 tasks

sobitneupane mentioned this pull request Apr 12, 2023

[HOLD for payment 2023-04-10] [$1000] Emoji picker moves to top left side when using Tab key #16384

Closed

6 tasks

sobitneupane mentioned this pull request Apr 21, 2023

[HOLD for payment 2023-04-18] [$4000] Emoji reaction button sizes are not 28px #15815

Closed

2 tasks

parasharrajat reviewed May 26, 2023

View reviewed changes

parasharrajat mentioned this pull request May 26, 2023

[HOLD for payment 2023-05-25] [$1000] Holding down on the plus emoji icon selects the next comment username #18661

Closed

6 tasks

hoangzinh reviewed Jan 22, 2024

View reviewed changes

hoangzinh mentioned this pull request Jan 22, 2024

[HOLD for payment 2024-01-17] [$250] Action menu - Double-clicking the "Add Reaction" icon highlights the text of the message. #33547

Closed

6 tasks

rushatgabhane reviewed Oct 2, 2024

View reviewed changes

rushatgabhane mentioned this pull request Oct 2, 2024

[$250] Emoji picker - Clicking on same emoji reaction changes skin tone instead of removing #46387

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add/remove reaction #15210

feat: add/remove reaction #15210

hannojg commented Feb 16, 2023 •

edited

Loading

stitesExpensify left a comment

stitesExpensify commented Mar 8, 2023

OSBotify commented Mar 8, 2023

0xmiros commented Mar 8, 2023

0xmiros Mar 8, 2023

stitesExpensify Mar 8, 2023

0xmiros commented Mar 8, 2023 •

edited

Loading

0xmiros commented Mar 8, 2023

OSBotify commented Mar 9, 2023

parasharrajat Mar 10, 2023

OSBotify commented Mar 10, 2023

parasharrajat Mar 29, 2023

puneetlath commented Apr 3, 2023

stitesExpensify commented Apr 5, 2023

sobitneupane commented Apr 12, 2023

sobitneupane commented Apr 21, 2023

parasharrajat May 26, 2023

hoangzinh Jan 22, 2024

rushatgabhane left a comment •

edited

Loading

feat: add/remove reaction #15210

feat: add/remove reaction #15210

Conversation

hannojg commented Feb 16, 2023 • edited Loading

Details

Fixed Issues

Tests

Quick reactions are working

The emoji picker is working

📩 Sending & receiving works

Adding the same reaction twice with different skin tone

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

stitesExpensify left a comment

Choose a reason for hiding this comment

stitesExpensify commented Mar 8, 2023

OSBotify commented Mar 8, 2023

0xmiros commented Mar 8, 2023

0xmiros Mar 8, 2023

Choose a reason for hiding this comment

stitesExpensify Mar 8, 2023

Choose a reason for hiding this comment

0xmiros commented Mar 8, 2023 • edited Loading

0xmiros commented Mar 8, 2023

OSBotify commented Mar 9, 2023

parasharrajat Mar 10, 2023

Choose a reason for hiding this comment

OSBotify commented Mar 10, 2023

parasharrajat Mar 29, 2023

Choose a reason for hiding this comment

puneetlath commented Apr 3, 2023

stitesExpensify commented Apr 5, 2023

sobitneupane commented Apr 12, 2023

sobitneupane commented Apr 21, 2023

parasharrajat May 26, 2023

Choose a reason for hiding this comment

hoangzinh Jan 22, 2024

Choose a reason for hiding this comment

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

hannojg commented Feb 16, 2023 •

edited

Loading

0xmiros commented Mar 8, 2023 •

edited

Loading

rushatgabhane left a comment •

edited

Loading