-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
New Branding - New Sign In Imagery #15737
Conversation
@shawnborton I have a couple of questions I didn't anticipate for the mobile screens-
|
Will update likely after ECX |
Let's plan to knock this out today! I can help prep all of the final assets we'll need. |
@grgia, can you please pull the main branch and resolve the conflicts? |
Done! @thesahindia |
@thesahindia this should be good for final review if you're able to prioritize it! |
Yeah, I am testing it right now. |
I think the password spacing is NAB for this PR. And Adhoc logo addressed here. |
Reviewer Checklist
Screenshots/Videos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well!
Just a note that on native you will see a crash. It was fixed recently. I tested the PR after pulling the main. @grgia, let's merge main again so that @AndrewGable doesn't face that problem while testing. Also I just wanted to point out that our code in sign-in flow components is a little bit messy. I think we should log a new GH to get it cleaned up. |
@thesahindia @AndrewGable merged main, final review! |
@AndrewGable looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
🤔 - The checklist test must have been running, but checklist was all filled out. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.3.1-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.3.1-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.1-3 🚀
|
Details
Add new sign in imagery as part of new branding project
Fixed Issues
$ #12253
Tests
Note for DEV:
To easily test without passwordless beta, go to
SignInPage.js
and replace allPermissions.canUsePasswordlessLogins(this.props.betas);
withPermissions.canUsePasswordlessLogins([]);
The correct flows for passwordless can be found in the screenshots section. The correct flows without passwordless beta can be found in this comment: #15737 (comment)
Likewise, to test beta, use
Permissions.canUsePasswordlessLogins(['passwordless']);
WEB / Desktop
All Platforms:
On log in page, visual QA
Type in email with 2FA enabled, visual QA Magic Code page on both small and wide screen
Type in magic code, visual QA 2FA page on both small and wide screen
Click "go back"
Type in email that does not exist as an account
visual QA Magic Code page on both small and wide screen
Go to password page, (to do this I hardcoded
showPasswordForm
as true and all others as false in signInPage.js), visual QA on both small and wide screenrepeat steps going offline for each page and ensure only one offline indicator shows.
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Web Sizes
Web
Web Small
Web New User
Mobile Web - Chrome
CHROME
Mobile Web - Safari
SAFARI
Desktop
Desktop Offline
iOS
IOS
IOS Offline
Android
ANDROID
ANDROID OFFLINE