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

Added cancel on ESC to the ConfirmModal #5741

Merged
merged 1 commit into from
Oct 12, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Oct 9, 2021

Details

Proposal => #5158 (comment)

I checked and verified that all ConfirmModal has a cancel handler.

Fixed Issues

$ #5158

Tests | QA Steps

  1. Navigate to a user.
  2. Add an attachment of more than 50 MB.
  3. Expect "attachment too large" warning to pop up.
  4. Press the Escape key.

Tested On

  • Web
  • Mobile Web (no External Keyboard)
  • Desktop
  • iOS (no External Keyboard)
  • Android (no External Keyboard)

Screenshots

Web | Desktop

output_file.mp4

Mobile Web

iOS

Android

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
@parasharrajat parasharrajat requested a review from a team as a code owner October 9, 2021 04:50
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team October 9, 2021 04:50
@parasharrajat parasharrajat changed the title Added cancel on ESC to the ConfirmModal [HOLD] Added cancel on ESC to the ConfirmModal Oct 11, 2021
Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

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

Happy to merge this post-freeze

@Luke9389
Copy link
Contributor

Actually, we might be fine to merge this now. It's not directly related to N6 but it is touching the N6 flows and will affect users who are trying out N6. What do you think @parasharrajat?

@parasharrajat
Copy link
Member Author

Yup, I was hoping the same @Luke9389. At last, It's your call.

@parasharrajat parasharrajat changed the title [HOLD] Added cancel on ESC to the ConfirmModal Added cancel on ESC to the ConfirmModal Oct 12, 2021
@parasharrajat parasharrajat changed the title Added cancel on ESC to the ConfirmModal [Hold] Added cancel on ESC to the ConfirmModal Oct 12, 2021
@Luke9389
Copy link
Contributor

OK go ahead and take it off hold. I think this is worth merging now.

@Luke9389 Luke9389 changed the title [Hold] Added cancel on ESC to the ConfirmModal Added cancel on ESC to the ConfirmModal Oct 12, 2021
@Luke9389
Copy link
Contributor

OH, I suppose I can do that :)

@Luke9389 Luke9389 merged commit 24cc6ff into Expensify:main Oct 12, 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 by @Luke9389 in version: 1.1.7-25 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

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.

None yet

3 participants