-
Notifications
You must be signed in to change notification settings - Fork 24
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
Get referral code from the web app when onboarding invitee #618
Get referral code from the web app when onboarding invitee #618
Conversation
📦 build.zip [updated at Oct 11, 1:46:58 PM UTC] |
import * as styles from './styles.module.css'; | ||
import { WebAppMessageHandler } from './WebAppMessageHandler'; | ||
|
||
const DEBUG_INVITEE_FLOW = 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.
Do not forget to remove before merge
} | ||
|
||
function setReferralCode(_referralCode: string) { | ||
// save referralCode |
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.
Here we also need not to safe incorrect code to avoid softlock state for the user who clicked on incorrect link
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.
Now referral code is checked on the webapp side, so it should be safe to just save it here as is
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.
I think, doublecheck will not harm :)
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.
Extension should be more reliable then web
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.
Updated ✅ Response will fail in case of incorrect referral code. The saveReferrer
function is commented out because its implementation is already a part of my other PR
: 'https://app.zerion.io' | ||
); | ||
|
||
enum WebAppCallbackMethod { |
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.
Aren't enums considered deprecated?
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, right, changed to just type
✅
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.
Are they?...
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.
Not really, some consider them bad, some don't... I think in this context its not a big deal: I just wanted to have some "sum"-type to enumerate possible callback methods
} | ||
} | ||
|
||
export function WebAppMessageHandler({ path }: { path: string }) { |
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.
let's use pathname
here?
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.
✅
return value as RemoteConfig[K]; | ||
}, | ||
useErrorBoundary: false, | ||
cacheTime: 0, |
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.
why?
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.
removed cacheTime: 0
, added staleTime: 8000
✅
return value as RemoteConfig[K]; | ||
}, | ||
useErrorBoundary: false, | ||
staleTime: 10000, |
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.
Do we need to provide suspense
here?
Hope this will not lead to ui blinking
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.
✅ Yep, let's have suspense: false
isLoading: isLoadingRemoteConfigValue, | ||
} = useRemoteConfigValue('extension_referral_program'); | ||
|
||
if (isLoadingRemoteConfigValue) { |
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.
If we use isLoading, we can set suspense: false
inside useQuery
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.
Thanks, updated ✅
|
||
const ZERION_WEB_APP_URL = new URL( | ||
process.env.NODE_ENV === 'development' | ||
? 'http://localhost:3000' |
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 like this should be a var inside .env file
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.
Agree, or just use hardcoded 'https://app.zerion.io'
, and localhost link shouldn't be commited
Whichever's easier
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.
I don't think we need https://localhost:3000
anymore (since the other PR is already merged) ✅ Updated
|
||
type WebAppCallbackMethod = 'set-referral-code'; | ||
|
||
interface WebAppMessage<T = unknown> { |
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.
We don't use generic value of this interface anywhere. But this is probably not a problem.
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.
Let's just have params?: unknown
for now ✅
To test this PR you'll need https://github.com/zeriontech/pulse-frontend/pull/3331