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

Thesahindia/UI/confirmView #7850

Merged
merged 11 commits into from
Mar 7, 2022
Merged

Thesahindia/UI/confirmView #7850

merged 11 commits into from
Mar 7, 2022

Conversation

thesahindia
Copy link
Member

@thesahindia thesahindia commented Feb 21, 2022

Details

  • Made confirmView component to reuse in confirmModal and confirmPopover
  • Made delete confirmModal consistent

Fixed Issues

$ #7545

Tests

  • Verify that no errors appear in the JS console

QA Steps

  1. Open app
  2. Navigate to Settings > Payments and try to delete any payment
  3. Navigate to Settings > Workspace and try to delete the workspace
  4. Navigate to a chat > Try to delete a comment
  5. Verify the confirmModal/Popover is consistent
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screenshot 2022-02-21 at 6 58 13 PM

Screenshot 2022-02-21 at 7 00 44 PM

Screenshot 2022-02-21 at 8 06 03 PM

Mobile Web

Screenshot 2022-02-21 at 6 49 56 PM

Screenshot 2022-02-21 at 8 35 02 PM

Screenshot 2022-02-21 at 9 50 03 PM

Desktop

Screenshot 2022-02-21 at 9 32 18 PM

Screenshot 2022-02-21 at 9 32 31 PM

Screenshot 2022-02-21 at 9 32 55 PM

iOS

Screenshot 2022-02-21 at 8 22 51 PM

Screenshot 2022-02-21 at 6 51 49 PM

Android

Screenshot 2022-02-21 at 6 45 01 PM

Screenshot 2022-02-21 at 6 45 36 PM

Screenshot 2022-02-21 at 8 13 50 PM

@thesahindia thesahindia requested a review from a team as a code owner February 21, 2022 16:53
@MelvinBot MelvinBot requested review from Beamanator and parasharrajat and removed request for a team February 21, 2022 16:53
@parasharrajat
Copy link
Member

Testing shortly...

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
parasharrajat
parasharrajat previously approved these changes Feb 24, 2022
Copy link
Member

@parasharrajat parasharrajat 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

🎀 👀 🎀 C+ reviewed

@parasharrajat
Copy link
Member

cc: @Beamanator

@Beamanator
Copy link
Contributor

Thanks @parasharrajat I'll check this out today, I was on vacation last week :D

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.

Overall looks great, just a few style changes and comment change suggestion

@Beamanator
Copy link
Contributor

Thanks for the quick edits @thesahindia ! 👍

@Beamanator Beamanator merged commit 9dd2a45 into Expensify:main Mar 7, 2022
@OSBotify
Copy link
Contributor

OSBotify commented Mar 7, 2022

✋ 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 @Beamanator in version: 1.1.42-0 🚀

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

@mvtglobally
Copy link

We are still able to repro #7768 in IOS, so limited QA possible. Rest platforms ok

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

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.

5 participants