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 signin page layout #2196

Merged
merged 49 commits into from
Apr 12, 2021
Merged

Fix signin page layout #2196

merged 49 commits into from
Apr 12, 2021

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented Apr 1, 2021

Details

The way the SigninPageLayout component was originally written was confusing and required bits and pieces of the forms that were its children to be located across separate components.

Since we eventually want to refactor the LoginForm to be a single component (without wide/narrow counterparts), I've refactored the SigninPageLayout to be easier to use with single component forms as children.

Fixed Issues

Expensify/Expensify#157816

QA Testing Steps

  1. Navigate to the sign in page.
  2. Verify that it looks the same as they do on production.
  3. Enter an email for an account that's already been created.
  4. Verify that the password entry page looks the same as it does on production.
  5. Create a new account with an email you have access to (I recommend using EmailOnDeck).
  6. Verify that resend validation link screen looks the same as it does on production.
  7. For Web, Mobile Web, iOS, and Android, use the link in the email that was sent to access the set password page.
  8. Verify that the set password page looks the same as it does on production.

Tested On

  • Web
  • Desktop
  • iOS
  • Mobile Web
  • Android

Screenshots

Web

Screen Shot 2021-04-08 at 11 18 34 AM

Screen Shot 2021-04-08 at 11 18 44 AM

Screen Shot 2021-04-08 at 11 18 58 AM

Screen Shot 2021-04-08 at 11 22 03 AM

Mobile Web

Screen Shot 2021-04-08 at 11 19 58 AM

Screen Shot 2021-04-08 at 11 19 39 AM

Screen Shot 2021-04-08 at 11 19 33 AM

Screen Shot 2021-04-08 at 11 19 24 AM

Desktop (set password page can't be accessed via deep links on desktop)

Screen Shot 2021-04-08 at 11 12 11 AM

Screen Shot 2021-04-08 at 11 12 07 AM

Screen Shot 2021-04-08 at 11 24 12 AM

iOS

Screen Shot 2021-04-08 at 11 06 57 AM

Screen Shot 2021-04-08 at 11 06 53 AM

Screen Shot 2021-04-08 at 11 07 11 AM

Screen Shot 2021-04-08 at 11 07 28 AM

Android

Screen Shot 2021-04-08 at 11 17 52 AM

Screen Shot 2021-04-08 at 11 18 27 AM

Screen Shot 2021-04-08 at 11 18 16 AM

Screen Shot 2021-04-08 at 11 20 34 AM

ctkochan22
ctkochan22 previously approved these changes Apr 2, 2021
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

Looking good. Had a few minor comments.

src/pages/signin/SignInPage.js Outdated Show resolved Hide resolved
src/pages/signin/SignInPage.js Outdated Show resolved Hide resolved
src/pages/signin/SignInPageLayout/SignInPageLayoutWide.js Outdated Show resolved Hide resolved
src/pages/signin/SignInPageLayout/SignInPageLayoutWide.js Outdated Show resolved Hide resolved
@shawnborton
Copy link
Contributor

Looks like the "Change Expensify login" blue link is not in the correct brand font. Can you fix that quickly?

@marcaaron
Copy link
Contributor

Navigate to the sign in page, the set password page, and the resend validation link pages.

Can we provide more detailed test steps here? It might not yet be clear to QA how to access these various screens.

Let's also test and add screenshots for these platforms:

  • Desktop
  • iOS
  • Android

@marcaaron
Copy link
Contributor

Not sure if this was introduced here or another PR, but the SetPassword screen is missing padding on iOS

2021-04-05_11-09-50

@jasperhuangg
Copy link
Contributor Author

jasperhuangg commented Apr 6, 2021

Looks like the "Change Expensify login" blue link is not in the correct brand font. Can you fix that quickly?

@shawnborton Which font is our brand font? It looks like for our styles we either default to a font or set it to GTA. These are the styles for that link in production:

https://github.com/Expensify/Expensify.cash/blob/0921feec98fa5d1ce14ee51071bebe9e148bccc8/src/styles/styles.js#L27-L30

@shawnborton
Copy link
Contributor

You would just need to simply add fontFamily: fontFamily.GTA, but at the same time, I realize this is totally not something you introduced so we can keep the scope slimmer and fix this in a separate PR.

@jasperhuangg
Copy link
Contributor Author

@shawnborton Sounds good, I'll make a new PR!

@jasperhuangg
Copy link
Contributor Author

@marcaaron I fixed the iOS display issue with the Set Password page and provided screenshots/instructions for each page for each platform. Should be good for another review, thanks!

@marcaaron
Copy link
Contributor

marcaaron commented Apr 8, 2021

Looks good. But we'll need to merge master on this guy since there is a failure.

@jasperhuangg
Copy link
Contributor Author

@marcaaron Looks like the tests are passing now after merging master, thanks for the review! Feel free to merge once you think everything checks out.

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

Changes look great but there's one more issue.

@marcaaron marcaaron merged commit 37543bd into master Apr 12, 2021
@marcaaron marcaaron deleted the jasper-fixSigninPageLayout branch April 12, 2021 15:49
@isagoico isagoico added DeployBlockerCash This issue or pull request should block deployment and removed DeployBlockerCash This issue or pull request should block deployment labels Apr 20, 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.

5 participants