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

Fix popovers after regression #2424

Merged
merged 2 commits into from
Apr 16, 2021
Merged

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Apr 15, 2021

cc @npsedhain @tgolen

Details

Fixing a regression caused by this PR

Fixed Issues

Fixes #2420

Tests / QA Steps (Web/Desktop)

  1. Hit the global create FAB, and verify that the CreateMenu appears in the correct position in the sidebar. It should fade in from the left and fade out to the left.
  2. Hit the + icon in the ReportActionCompose. Verify that the CreateMenu appears in the correct position above the compose input. It should fade in from the bottom and fade out to the bottom.
  3. Go to Settings -> Profile -> Edit Photo. Verify that the CreateMenu appears in the correct position above the "Edit Photo" button. It should fade in from the right and fade out to the right.
  4. Right click on a message, and verify that the ReportActionContextMenu appears as it should in the location where the user clicks.
  5. Open the emoji picker, make sure it looks normal (it shouldn't have any noticeable animation when it opens and closes).

Tests / QA Steps (mWeb/iOS/Android)

  1. Hit the following buttons, and verify that the CreateMenu appears as a bottom-docked modal:
    1. Global create FAB
    2. + in the ReportActionCompose
    3. Settings -> Profile -> Edit Photo
    4. Long-press a message.
    5. The emoji picker

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Menu Web Mobile Web Desktop iOS Android
Global Create image image image image image
ReportActionCompose image image image image image
Edit Photo image image image image image
ReportActionContextMenu image image image image image
Emoji Picker image image image

@roryabraham roryabraham requested a review from a team April 15, 2021 20:06
@roryabraham roryabraham self-assigned this Apr 15, 2021
@MelvinBot MelvinBot removed the request for review from a team April 15, 2021 20:06
@github-actions
Copy link
Contributor

github-actions bot commented Apr 15, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@MelvinBot MelvinBot requested a review from madmax330 April 15, 2021 20:06
@roryabraham roryabraham changed the title Fix popovers after regression [WIP] Fix popovers after regression Apr 15, 2021
@roryabraham
Copy link
Contributor Author

WIP while I gather screenshots

@roryabraham roryabraham changed the title [WIP] Fix popovers after regression Fix popovers after regression Apr 15, 2021
@roryabraham
Copy link
Contributor Author

recheck

@roryabraham
Copy link
Contributor Author

Hmmm CLA is being weird ... commenting "recheck" did restart the CLA action which passed here, but for some reason it didn't update the PR. When I manually retried the check from the PR, it failed just like the first time due to the problem fixed in this PR

@roryabraham roryabraham mentioned this pull request Apr 16, 2021
5 tasks
# Conflicts:
#	src/pages/home/report/ReportActionCompose.js
@roryabraham roryabraham requested a review from a team as a code owner April 16, 2021 01:15
@MelvinBot MelvinBot requested review from Beamanator and removed request for a team April 16, 2021 01:15
@roryabraham
Copy link
Contributor Author

Retested after merging main (and added the new EmojiPickerMenu popover), and verified that this still fixes the popover issues.

@npsedhain
Copy link
Contributor

hey @roryabraham, thanks so much for this, also I am sorry I didn't see this yesterday, it was late in the night this part of the world. Let me know if there is anything I can do.

@Beamanator
Copy link
Contributor

Looks like you may be missing some screenshots for the emoji picker in android & desktop? I took these to help:

Desktop:
Screen Shot 2021-04-16 at 12 44 11 PM

Android:
Screen Shot 2021-04-16 at 12 42 42 PM

@roryabraham
Copy link
Contributor Author

Thanks for testing and gathering those screenshots @Beamanator !

@Beamanator Beamanator self-requested a review April 16, 2021 16:05
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.

Love it - #LGTM 👍

@roryabraham
Copy link
Contributor Author

@madmax330 I'm going to go ahead and merge this since it resolves a deploy blocker.

@roryabraham roryabraham merged commit 02c6988 into main Apr 16, 2021
@roryabraham roryabraham deleted the Rory-FixReportActionContextMenuPopover branch April 16, 2021 16:12
@isagoico
Copy link

Attachment button in iOS is not working as expected, keyboard opens instead of modal

Expected Result:

Popup " Add attachment" opens

Actual Result:

Popup flashes for a moment and keypad opens to enter the text

Action Performed:

  1. Launch the app and login
  2. Tap any chat from LHN
  3. Tap + to attach a photo

Workaround:

Unknown

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android
Desktop App
Mobile Web

**Version Number:**1.0.24-.

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

attachment.mp4

@isagoico isagoico added the DeployBlockerCash This issue or pull request should block deployment label Apr 17, 2021
@Beamanator
Copy link
Contributor

I reproduced on your branch - I often forget to test with keyboard enabled, so great catch @isagoico 👍

@Beamanator
Copy link
Contributor

I believe this is the problem - https://github.com/Expensify/Expensify.cash/blob/main/src/pages/home/report/ReportActionCompose.js#L294, but supposedly that was added to fix some modal 2 modal transition in another PR

@roryabraham
Copy link
Contributor Author

Thanks for looking into this @Beamanator. I will fiddle around and try to fix it. This is why it's always better to test with a physical device...

@roryabraham
Copy link
Contributor Author

Fix is here ... @isagoico are you sure this isn't present on production? I do not think this PR caused the bug/regression described.

@OSBotify
Copy link
Contributor

🚀 [Deployed](https://github.com/Expensify/Expensify.cash
/actions/runs/765130740) 🚀 to
staging on Mon Apr 19 2021 at 22:41:56 GMT+0000 (Coordinated Universal Time)

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

@isagoico
Copy link

@roryabraham Sorry that I missed this before. I just verified with a tester that this is not reproducible in prod.

@roryabraham
Copy link
Contributor Author

roryabraham commented Apr 23, 2021

Just confirmed on iOS v1.0.29-0 that this deploy blocker is solved.

@roryabraham roryabraham removed the DeployBlockerCash This issue or pull request should block deployment label Apr 23, 2021
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.

Copy - Copy banner when using right click button on message too large
6 participants