Skip to content

fix: message highlight issue & mini context-menu issue#5270

Merged
jasperhuangg merged 3 commits into
Expensify:mainfrom
parasharrajat:row-hightlight
Sep 22, 2021
Merged

fix: message highlight issue & mini context-menu issue#5270
jasperhuangg merged 3 commits into
Expensify:mainfrom
parasharrajat:row-hightlight

Conversation

@parasharrajat

@parasharrajat parasharrajat commented Sep 15, 2021

Copy link
Copy Markdown
Member

@jasperhuangg Please review.

Details

#5197 (comment)
#5250 (comment)

Fixed Issues

$ #5197
$ #5250

Tests | QA Steps

  1. Open any chat.
  2. Right-click on any message. When Context Menu is open, no mini menu is shown.
  3. Now move to the empty area on the same message, right-click on that empty area(which is not covered by the context menu).
  4. Click anywhere on the app. Context Menu should close.
  5. Now hover back on the same message. The mini menu should be shown again.

  1. Long press/click over any chat item,
  2. Chat options like Copy to Clipboard or Mark as Unread. will be shown.
  3. Choose any one of the options mentioned above.
  4. Hover the cursor on another message.
  5. Previous message should not be highlighted anymore.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | Desktop

fix-row-f.mp4

Mobile Web

fix-row-f.mp4

iOS

Android

fix-row-f.mp4

@parasharrajat parasharrajat requested a review from a team as a code owner September 15, 2021 17:07
@MelvinBot MelvinBot requested review from luacmartins and removed request for a team September 15, 2021 17:07
@luacmartins

Copy link
Copy Markdown
Contributor

It seems that there is still some sort of race condition between the callbacks. When trying to right click through multiple messages in a short period of time, we can still get in some weird state. It's a bit of an edge case, but it would be nice to fix it.

test.mov

@parasharrajat

Copy link
Copy Markdown
Member Author

Sure, let me check this.

@kadiealexander

Copy link
Copy Markdown
Contributor

Hi @parasharrajat, I've placed this issue on hold as per this update, as we are prioritising issues related to a feature release scheduled for 9/24. As an apology for the delay, we will add a $100 bonus as a thank you for waiting.

@parasharrajat

Copy link
Copy Markdown
Member Author

@luacmartins I have handled three types of possible race conditions now. It is performing better.

  1. When onHide callback was getting overridden, if a new context menu was opening.
  2. When onHide callback (called after user has done some action) was getting overridden, if a new context menu was opening.
  3. When the Previous instance of the menu is supposed to close with a delay(If happens when user click either copy to clipboard or mark unread), but it was closing the new instance if user opened a new context menu.

@luacmartins

Copy link
Copy Markdown
Contributor

This is a tricky one. I'm still seeing the race condition. I'm not too sure how to solve those though. Do you have any more ideas?

test.mov

@parasharrajat

Copy link
Copy Markdown
Member Author

Hmm. Interesting! I specifically checked all callbacks and applied for the fix. I am not facing any with the new changes. But it does not mean that there are none. I will check again.

@jasperhuangg jasperhuangg self-requested a review September 21, 2021 17:42
@jasperhuangg

Copy link
Copy Markdown
Contributor

@parasharrajat Let me know when you're done fixing that race condition, or if you need some help investigating!

@parasharrajat

Copy link
Copy Markdown
Member Author

@jasperhuangg Yeah, I need your help to confirm if race conditions are still happening on this PR. I don't see them anymore. Maybe my computer is processing too fast.

@jasperhuangg jasperhuangg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests well on all platforms!

@jasperhuangg jasperhuangg merged commit 5f51222 into Expensify:main Sep 22, 2021
@OSBotify

Copy link
Copy Markdown
Contributor

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

@luacmartins

Copy link
Copy Markdown
Contributor

Hmm I thought this PR was on n6-hold and we should not merge it. Is that not the case?

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @jasperhuangg in version: 1.1.1-9 🚀

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

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @timszot in version: 1.1.2-7 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants