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

feat: add keyless backup into onboarding flow #5591

Merged
merged 37 commits into from
Jul 10, 2024
Merged

feat: add keyless backup into onboarding flow #5591

merged 37 commits into from
Jul 10, 2024

Conversation

MuckT
Copy link
Collaborator

@MuckT MuckT commented Jun 28, 2024

Description

Adds the keyless backup flow into onboarding with a redesigned SignInWithEmail.tsx screen specifically for onboarding. Note the onboarding background colors are fixed in #5592 and the below videos use a throwaway number and seed phrase.

iOS CAB iOS Recovery Phrase iOS Skip
Screen.Recording.2024-07-01.at.5.09.36.PM.mov
Screen.Recording.2024-07-01.at.4.41.22.PM.mov
Screen.Recording.2024-07-01.at.4.42.31.PM.mov
Android CAB Android Recovery Phrase Android Skip
Screen.Recording.2024-07-02.at.5.24.17.PM.mov
Screen.Recording.2024-07-02.at.5.16.34.PM.mov
Screen.Recording.2024-07-02.at.5.18.15.PM.mov

Test plan

  • Tested locally on iOS
  • Tested locally on Android
  • Unit tests updated

Related issues

Backwards compatibility

Yes

Network scalability

N/A

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 87.32394% with 9 lines in your changes missing coverage. Please review.

Project coverage is 86.73%. Comparing base (2e02e91) to head (ca8a587).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5591      +/-   ##
==========================================
+ Coverage   86.72%   86.73%   +0.01%     
==========================================
  Files         773      774       +1     
  Lines       31567    31626      +59     
  Branches     5456     5182     -274     
==========================================
+ Hits        27375    27430      +55     
- Misses       3963     4151     +188     
+ Partials      229       45     -184     
Files Coverage Δ
branding/celo/src/images/email.png 100.00% <ø> (ø)
src/analytics/Events.tsx 100.00% <100.00%> (ø)
src/analytics/Properties.tsx 100.00% <ø> (ø)
src/images/Images.ts 100.00% <100.00%> (ø)
src/keylessBackup/SignInWithEmail.tsx 98.92% <100.00%> (+0.67%) ⬆️
...boarding/registration/OnboardingRecoveryPhrase.tsx 86.79% <100.00%> (+0.25%) ⬆️
test/values.ts 100.00% <ø> (ø)
src/account/AccountKeyEducation.tsx 75.00% <0.00%> (-5.00%) ⬇️
src/onboarding/steps.ts 91.04% <62.50%> (-0.49%) ⬇️

... and 76 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e02e91...ca8a587. Read the comment docs.

src/navigator/types.tsx Outdated Show resolved Hide resolved
@MuckT MuckT marked this pull request as ready for review July 3, 2024 00:29
typeof title === 'string' ? <Text style={headerStyles.headerTitle}>{title}</Text> : title
function CustomHeader({ left, right, title, style, subTitle }: Props) {
const titleComponent = subTitle ? (
<HeaderTitleWithSubtitle title={title} subTitle={subTitle} />
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the extra subtitle option here? Instead can the consumers just pass <HeaderTitleWithSubtitle title={title} subTitle={subTitle} />, since title already accepts ReactNode as a prop?

Copy link
Collaborator Author

@MuckT MuckT Jul 8, 2024

Choose a reason for hiding this comment

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

Fixed in 32b452a 85474bc!

src/keylessBackup/types.ts Outdated Show resolved Hide resolved
const onboardingProps = useSelector(onboardingPropsSelector)
const { step, totalSteps } = getOnboardingStepValues(Screens.ProtectWallet, onboardingProps)
// Use a lower step count for CAB onboarding
Copy link
Contributor

Choose a reason for hiding this comment

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

what does "lower" mean here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's similar to what was done in here where we use a different screen to have a consistent step count.

src/keylessBackup/SignInWithEmail.tsx Outdated Show resolved Hide resolved
Comment on lines 102 to 120
const onPressContinue = () => {
ValoraAnalytics.track(KeylessBackupEvents.cab_setup_recovery_phrase)
bottomSheetRef.current?.close()
goToNextOnboardingScreen({
firstScreenInCurrentStep: Screens.SignInWithEmail,
onboardingProps: { ...onboardingProps, showRecoveryPhraseEducation: true },
})
}

const onPressSkip = () => {
ValoraAnalytics.track(KeylessBackupEvents.cab_sign_in_with_email_screen_skip, {
keylessBackupFlow,
origin,
})
goToNextOnboardingScreen({
firstScreenInCurrentStep: Screens.SignInWithEmail,
onboardingProps,
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move this to the bottom sheet componet since its only used there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed in 685f385!

finishOnboarding(Screens.ChooseYourAdventure)
} else {
// DO NOT CLEAR NAVIGATION STACK HERE - breaks restore flow on initial app open in native-stack v6
navigate(Screens.VerificationStartScreen)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the videos, looks like after backup is successful, the flow navigates to home directly, is that expected? Per this, looks like it should go to phone verification / CYA?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is fixed in #5611.

forwardedRef={bottomSheetRef}
title={t('signInWithEmail.bottomSheet.title')}
titleStyle={styles.bottomSheetTitle}
testId="KeylessBackupSignInWithEmail/HelpInfoBottomSheet"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is not help info right? could use a better name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 0320f30!

@@ -124,8 +245,15 @@ const styles = StyleSheet.create({
scrollContainer: {
padding: Spacing.Thick24,
},
header: {
paddingHorizontal: variables.contentPadding,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is 16 right? seems like the scroll container uses 24, should this be consistent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to be consistent with headers in the previous screens which appear to be 16.

src/components/header/CustomHeader.tsx Outdated Show resolved Hide resolved
src/onboarding/steps.ts Outdated Show resolved Hide resolved
Comment on lines 270 to 273
if (showRecoveryPhraseEducation) {
navigate(Screens.AccountKeyEducation, {
origin: 'cabOnboarding',
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove this case and directly navigate to this screen from SignInWithEmail. Otherwise there is an edge case where our step counter fn could crash when it walks thru graph since AccountKeyEducation does not have a next step. Although practically this may never happen since showRecoveryPhraseEducation will be false, but anyways good to keep this clean.

Comment on lines 50 to 53
goToNextOnboardingScreen({
firstScreenInCurrentStep: Screens.SignInWithEmail,
onboardingProps: { ...onboardingProps, showRecoveryPhraseEducation: true },
})
Copy link
Contributor

Choose a reason for hiding this comment

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

per comment in steps, this can just navigate directly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 0385484!

src/components/header/CustomHeader.tsx Outdated Show resolved Hide resolved
@MuckT MuckT added this pull request to the merge queue Jul 10, 2024
Merged via the queue into main with commit 710b2c9 Jul 10, 2024
16 checks passed
@MuckT MuckT deleted the tomm/act-1219 branch July 10, 2024 03:36
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.

2 participants