-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Implement unvalidated signups for newDot #42887
Conversation
@ Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: mWeb Chromemy sim isn't working for mweb chrome iOS: NativeScreen.Recording.2024-06-04.at.18.49.02.moviOS: mWeb SafariScreen.Recording.2024-06-04.at.18.46.42.movMacOS: DesktopScreen.Recording.2024-06-04.at.18.52.40.mov |
@srikarparsi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
🎯 @rushatgabhane, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #43039. |
@techievivek for the copy here, where did that come from? I don't think we need the second sentence, and we can just match exactly what is on OldDot for consistency. OldDot ref below: CC: @jamesdeanexpensify for a second opinion on that. Also, @shawnborton @dannymcclain @dubielzyk-expensify is the |
sounds good to me re:copy @trjExpensify ! |
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.
src/pages/signin/SignInPage.tsx
Outdated
const userLogin = Str.removeSMSDomain(credentials?.login ?? ''); | ||
|
||
// replacing spaces with "hard spaces" to prevent breaking the number | ||
const userLoginToDisplay = Str.isSMSLogin(userLogin) ? formatPhoneNumber(userLogin).replace(/ /g, '\u00A0') : userLogin; |
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.
NAB since you didn't add this logic to begin with, but can we move the .replace(/ /g, '\u00A0')
inside of formatPhoneNumber
?
@trjExpensify To confirm, we need |
Only on small screens here where we don't have So on small screens that would be:
On large screens it's:
|
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.
@carlosmiceli Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Screen recordings look good to me 👍 |
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.
Looks great, approving but not merging while we wait on the web PR
@@ -200,6 +217,11 @@ function SignInPageInner({credentials, account, activeClients = [], preferredLoc | |||
let welcomeText = ''; | |||
const headerText = translate('login.hero.header'); | |||
|
|||
const userLogin = Str.removeSMSDomain(credentials?.login ?? ''); | |||
|
|||
// replacing spaces with "hard spaces" to prevent breaking the number |
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.
NAB: You don't need this comment (or surrounding whitespace) since this change is occurring within formatPhoneNumber
now
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.
code looks good to me thank you for the work Vivek!
WebPR is on prod, let's take this off hold and merge it so it gets to staging with the app deploy later. :) |
Merging now since the web-e changes are on prod and this has been already approved. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@techievivek this PR appears to have caused this deploy blocker because users are getting booted out after validating their logins in app |
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.81-11 🚀
|
Hold until https://github.com/Expensify/Web-Expensify/pull/42216 is deployed to PROD.
Details
This adds the unvalidated signup on newDot. For a new user who is not on a controlled domain when they signup we directly take them to the product. No need of any magic code for the signup process.
Fixed Issues
Part of #30794
PROPOSAL: N/A
Tests
New signup flow testing
<email>, it's always great to see a new face around here! Please continue by tapping the join button below.
back
button.Existing account signup flow
Domain control account testing
Login with a 2fa account [Only on Staging or PROD]
Offline tests
QA Steps
Same as above.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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./** 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
)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-06-03.at.11.54.49.AM.mov
MacOS: Desktop
desktop-test.mov