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.

Sign up for GitHub

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

Jump to bottom

Refactor resend link page #5116

Merged
merged 13 commits into from
Oct 5, 2021
Merged

Refactor resend link page #5116

merged 13 commits into from
Oct 5, 2021

Conversation

MonilBhavsar
Copy link
Contributor

@MonilBhavsar MonilBhavsar commented Sep 7, 2021 •
edited
Loading

Details

Pass shouldShowWelcomeText prop and conditionally display welcome text - "Welcome to the New Expensify! Enter your phone number or email to continue."

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/176333
cc @michelle-thompson

Tests / QA

  1. Go to login page, make sure welcome text is displayed.
  2. Go to password page, make sure welcome text is displayed.
  3. Click "Forgot?", make sure welcome text is not displayed and you notice new layout change as in screenshots below
  4. Entered a new "Email/phone" and make sure welcome text is not displayed and you notice new layout change as in screenshots below

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen.Recording.2021-09-17.at.6.26.24.PM.mov

Mobile Web

Screenshot 2021-09-17 at 6 43 31 PM

Desktop

Screenshot 2021-09-17 at 6 54 06 PM

iOS

Simulator Screen Shot - iPhone 12 - 2021-09-29 at 23 25 46
Simulator Screen Shot - iPhone 12 - 2021-09-29 at 23 26 05
Simulator Screen Shot - iPhone 12 - 2021-09-29 at 23 27 06

Android

Screenshot_1631885445

@MonilBhavsar MonilBhavsar self-assigned this Sep 7, 2021
@MonilBhavsar MonilBhavsar marked this pull request as ready for review September 7, 2021 09:50
@MonilBhavsar MonilBhavsar requested a review from a team as a code owner September 7, 2021 09:50
@MelvinBot MelvinBot requested review from Beamanator and removed request for a team September 7, 2021 09:50
Beamanator
Beamanator requested changes Sep 7, 2021
View reviewed changes
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.

A few tiny changes requested, otherwise looks great 👍

Tiny question: In the design (specifically this one) it looks like "Email" has been changed to "Email address" - is that an expected change or no? Maybe that was accidentally changed in their screenshots

@MonilBhavsar MonilBhavsar changed the title Remove inaccurate copy on resend link page [HOLD] Remove inaccurate copy on resend link page Sep 8, 2021
@MonilBhavsar MonilBhavsar changed the title [HOLD] Remove inaccurate copy on resend link page Refactor resend link page Sep 17, 2021
@MonilBhavsar MonilBhavsar requested a review from a team September 17, 2021 13:34
@MelvinBot MelvinBot requested review from marcochavezf and removed request for a team September 17, 2021 13:34
@MonilBhavsar
Copy link
Contributor Author

@Beamanator, Off hold and requesting another reviewer too

@Beamanator
Copy link
Contributor

To me, it's a bit weird seeing This account exists but isn't validated, please check your inbox for the validation link. immediately after signing up with a new account. Made a note in the issue, will continue reviewing in case we decide not to change this :D

Beamanator
Beamanator previously approved these changes Sep 18, 2021
View reviewed changes
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.

Some tiny comment changes requested, all NAB - Also left comments about some text in the related issue, so won't merge yet

marcochavezf
marcochavezf previously approved these changes Sep 20, 2021
View reviewed changes
Copy link
Contributor

@marcochavezf marcochavezf left a comment

Choose a reason for hiding this comment

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

LGTM

@Jag96 Jag96 mentioned this pull request Sep 21, 2021
Closed
Jag96
Jag96 reviewed Sep 21, 2021
View reviewed changes
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

I'm seeing a RN warning for this usage:

App/src/pages/SetPasswordPage.js

Line 108 in fc85568

<SignInPageLayout welcomeText={this.props.translate('setPasswordPage.passwordFormTitle')}> 

react.development.js:220 Warning: Failed prop type: The prop `shouldShowWelcomeText` is marked as required in `SignInPageLayout`, but its value is `undefined`.

@MonilBhavsar
Copy link
Contributor Author

@Jag96 thanks for heads up, it will be resolved in this PR

Jag96
Jag96 reviewed Sep 23, 2021
View reviewed changes
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Looks good, holding off on approval/merge since this is polish

@MonilBhavsar
Copy link
Contributor Author

Ready for final review 🙌

marcochavezf
marcochavezf previously approved these changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@marcochavezf marcochavezf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍🏽

New view:

Screen Shot 2021-09-29 at 15 24 02

Old view (tested with current main branch):

Screen Shot 2021-09-29 at 15 17 29

Jag96
Jag96 reviewed Sep 29, 2021
View reviewed changes
Jag96
Jag96 approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

LGTM

@MonilBhavsar
Copy link
Contributor Author

friendly bump!

@Beamanator
Copy link
Contributor

Looks like this isn't on hold for anything, so we can merge yeah?

@Jag96 Jag96 merged commit 2202cc2 into main Oct 5, 2021
@Jag96 Jag96 deleted the monil-resendLinkCopy branch October 5, 2021 17:23
@OSBotify
Copy link
Contributor

OSBotify commented Oct 5, 2021

✋ 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 OSBotify mentioned this pull request Oct 6, 2021
Closed
13 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Oct 6, 2021

🚀 Deployed to staging by @Jag96 in version: 1.1.5-2 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 7, 2021

🚀 Deployed to production by @chiragsalian in version: 1.1.6-0 🚀

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

@chiragsalian chiragsalian mentioned this pull request Nov 3, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Beamanator Beamanator Beamanator approved these changes

@Jag96 Jag96 Jag96 approved these changes

@marcochavezf marcochavezf Awaiting requested review from marcochavezf

Assignees

@MonilBhavsar MonilBhavsar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MonilBhavsar @Beamanator @OSBotify @Jag96 @marcochavezf