-
Notifications
You must be signed in to change notification settings - Fork 8
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
Handle already used phone in UI #866
base: throw-error-when-exisitng-user-uses-otp
Are you sure you want to change the base?
Handle already used phone in UI #866
Conversation
Playwright ReportSummary
Suitesaccount-rewards.onboarded.spec.tscan visit rewards page
account-sendtag-checkout.onboarded.spec.tscan visit checkout page
can add a pending tag
cannot add an invalid tag name
can confirm a tag
can refer a tag
can refer multiple tags in separate transactions
cannot confirm a tag without paying
cannot add more than 5 tags
account-settings-backup.onboarded.spec.tscan backup account
can remove a signer
account.logged-in.spec.tscan visit account page
can update profile
activity.onboarded.spec.tscan visit activity page and see correct activity feed
can search on activity page
home.onboarded.spec.tscan visit token detail page
leaderboard.logged-in.spec.tscan visit leaderboard page
onboarding.logged-in.spec.tscan visit onboarding page
profile.anon.spec.tsanon user can visit public profile
anon user cannot visit private profile
profile.logged-in.spec.tslogged in user needs onboarding before visiting profile
profile.onboarded.spec.tscan visit other user profile and send by tag
can visit my own profile
can visit private profile
can view activities between another profile
send.onboarded.spec.tscan send USDC starting from profile page
can send USDC using tag starting from home page
can send USDC using sendid starting from home page
can send USDC using address starting from home page
can send ETH starting from profile page
can send ETH using tag starting from home page
can send ETH using sendid starting from home page
can send ETH using address starting from home page
can send SPX starting from profile page
can send SPX using tag starting from home page
can send SPX using sendid starting from home page
can send SPX using address starting from home page
can send SEND starting from profile page
can send SEND using tag starting from home page
can send SEND using sendid starting from home page
can send SEND using address starting from home page
sign-in.anon.spec.tsredirect on sign-in
redirect to send confirm page on sign-in
sign-up.anon.spec.tscan sign up
country code is selected based on geoip
|
a67c727
to
487007b
Compare
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.
Changes look good, a couple touchups here and there.
I would have preferred you split moving the functions and modifying the components into separate PRs so it's easier to pick out the logic from the refactors
)} | ||
</SchemaForm> | ||
)} | ||
{!form.formState.isSubmitSuccessful && !shouldShowBackUpPrompt && signUpForm} |
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.
Good job on this. Would prefer it to use a switch(true)
to make it even more readable.
Here's an example
case !isSendingUnlocked: |
packages/api/src/routers/_app.ts
Outdated
@@ -1,6 +1,6 @@ | |||
import type { inferRouterInputs, inferRouterOutputs } from '@trpc/server' | |||
import { createTRPCRouter } from '../trpc' | |||
import { authRouter } from './auth' | |||
import { authRouter } from './auth/router' |
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.
Is there a specific reason to change this route?
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.
yes, I added file for types for auth router and moved both files to auth directory. The reason types cannot be in the same file as router is that it cannot be imported in UI, just like RecoveryOptions
const { mutateAsync: validateSignatureMutateAsync } = api.challenge.validateSignature.useMutation( | ||
{ retry: false } | ||
) | ||
|
||
const handleSignIn = async () => { | ||
const handleSignIn = async (isNumberAlreadyUsed = false) => { |
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.
NIT: maybe should be isPhoneAlreadyUsed
to maintain consistency
<Paragraph size="$2" color="$color11"> | ||
Already have an account? | ||
</Paragraph> | ||
<SubmitButton onPress={() => handleSignIn()} disabled={isSigningIn} unstyled> |
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.
dont have to make this an arrow function since it has no args
onPress={handleSignIn}
is there a reason for the change
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.
actually there is a reason I did that, on press is passing event as a arg to the function, and this function have optional arg, typescript complains about types mismatch between event and function param, and it's right
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.
Ah ok I see. Looks good
<ButtonText>YES</ButtonText> | ||
</SubmitButton> | ||
<Button | ||
borderColor="$primary" |
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.
for light mode primary is always assigned to black. Unfortunately our tamagui doesn't do this for us yet
borderColor="$primary" | |
borderColor="$primary" | |
$theme-light={{borderColor: "$color12"}} |
const errorMessage = error.message.toLowerCase() | ||
form.setError('phone', { type: 'custom', message: errorMessage }) | ||
} | ||
} | ||
|
||
function handleBackUpConfirm() { | ||
const formData = form.getValues() | ||
void signUpWithPhone(formData, true) |
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.
NIT: This boolean is a bit magical. Hard to know what it does. Consider using a string enum or passing it in a object so we can name it
}) | ||
|
||
if (error) { | ||
if (status === AuthStatus.PhoneAlreadyUsed) { | ||
await handleSignIn(true) |
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.
See NIT below
w="$12" | ||
onPress={handleBackUpDenial} | ||
> | ||
<Button.Text color="$white" $gtMd={{ color: '$color12' }}> |
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.
This won't work on light mode. Instead use our radix-style color pallette
<Button.Text color="$white" $gtMd={{ color: '$color12' }}> | |
<Button.Text color="$color12"> |
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.
Found a bug
const { mutateAsync: validateSignatureMutateAsync } = api.challenge.validateSignature.useMutation( | ||
{ retry: false } | ||
) | ||
|
||
const handleSignIn = async () => { | ||
const handleSignIn = async (isNumberAlreadyUsed = false) => { | ||
setIsSigningIn(true) |
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.
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.
on it
Vercel Unique URL: https://sendapp-3bqw4aw69-0xsend.vercel.app |
UI part of the ticket https://3.basecamp.com/5743564/buckets/36023883/todos/8021528909
checked out off #864