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

Context Menu rendering #3515

Merged
merged 13 commits into from
Jun 16, 2021
Merged

Context Menu rendering #3515

merged 13 commits into from
Jun 16, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jun 10, 2021

Details

Fixed Issues

Fixes #2967

Tests | QA Steps (Web | desktop)

  1. Open Any conversation on E.cash.
  2. Open the Context menu on any message.
  3. Now try to open the context menu for another message. The previous message should close and a new one should open.
  4. Open the context menu on any message.
  5. Click somewhere outside the context menu. A context menu should close.

Tests | QA Steps (Mobile | Mweb)

  1. open Any conversation on E.cash
  2. Hold press any message to open the context menu.
  3. Click in the empty area should close the menu.
  4. There should be no apparent changes and the context menu should behave as before.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

context-menu-w.mp4

Mobile Web

1623342349136.mp4

Desktop

context-menu-d.mp4

iOS

Android

1623342349131.mp4

@parasharrajat parasharrajat requested a review from a team as a code owner June 10, 2021 16:12
@MelvinBot MelvinBot requested review from Dal-Papa and removed request for a team June 10, 2021 16:13
Dal-Papa
Dal-Papa previously approved these changes Jun 10, 2021
Copy link

@Dal-Papa Dal-Papa left a comment

Choose a reason for hiding this comment

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

This LGTM but I'd rather have @marcaaron or @Julesssss review the code than me.

@Julesssss
Copy link
Contributor

Sorry, I'm unable to take additional reviews today.

@parasharrajat
Copy link
Member Author

@marcaaron any thoughts. Do you see any gotchas to component composition here?

@marcaaron
Copy link
Contributor

Do you see any gotchas to component composition here?

Hmm sorry, can you ask that in another way? I'm not so sure what you meant.

@parasharrajat
Copy link
Member Author

Sorry, Could you please review it, and do you think is there any better way of reusing the popover as I feel we have added a couple of wrappers around the baseModal.

@marcaaron
Copy link
Contributor

Perhaps request in the Slack channel to see if someone else can provide a second opinion?

@parasharrajat
Copy link
Member Author

@Beamanator All yours. Thanks.

src/components/ContextMenuPopover/index.native.js Outdated Show resolved Hide resolved
src/components/ContextMenuPopover/index.native.js Outdated Show resolved Hide resolved
src/components/Modal/index.js Outdated Show resolved Hide resolved
src/components/Modal/index.js Outdated Show resolved Hide resolved
src/CONST.js Outdated Show resolved Hide resolved
src/components/ContextMenuPopover/index.js Outdated Show resolved Hide resolved
src/components/Modal/index.js Outdated Show resolved Hide resolved
src/styles/getModalStyles.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member Author

Full refactor. Thanks.

Beamanator
Beamanator previously approved these changes Jun 15, 2021
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Looks awesome, tested and works well 👍 Will let Rory check it before merging

roryabraham
roryabraham previously approved these changes Jun 15, 2021
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Nice, looks great!

@Beamanator Beamanator self-requested a review June 16, 2021 10:03
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

#LGTM!

@Beamanator Beamanator merged commit 3ebf613 into Expensify:main Jun 16, 2021
@OSBotify
Copy link
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.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.69-1🚀

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

@isagoico
Copy link

isagoico commented Jun 18, 2021

@parasharrajat We were still able to reproduce this issue #3636 in mWeb. Should we check this PR off? It was a pass in all the other platforms. (and seems this issue will be fixed in #3635

@parasharrajat
Copy link
Member Author

@isagoico Yeah there as accidentally bad merge of two PR which resulted in this behaviour. You are correct that other PR is going yo fix all the issues with context menu. I believe it will be merge today. So let's wait. Otherwise you can check it off, if that is a good option at the moment.

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.73-3🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 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.

[HOLD for payment 23 June] Web - Chat - Adjust modal rendering
8 participants