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(onboarding): remove steps on phone verification screen when importing #4834

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

satish-ravi
Copy link
Contributor

Description

Removes steps on phone verification screen when importing. Refactored the screen to accept a property isOnboarding instead of hideOnboardingStep which was used to identify whether the user is in the onboarding flow and whether to show steps or not.
Also fixed a bug where iOS gestures where disabled when not in onboarding instead of onboarding

Test plan

Unit tests, manual

Phone Input (onboarding) Verification Code input (onboarding) Phone Input (Settings)

Related issues

Backwards compatibility

Yes

// Disable iOS back during onboarding
gestureEnabled: route.params?.hideOnboardingStep ? false : true,
gestureEnabled: !route.params?.isOnboarding,
Copy link
Contributor Author

@satish-ravi satish-ravi Jan 31, 2024

Choose a reason for hiding this comment

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

This condition is flipped now. I believe this was buggy originally as we wanted to disable back gesture during onboarding but this was actually disabling it when not in onboarding, cc @MuckT (originally added in #3004)

@@ -23,12 +23,10 @@ import { StatsigExperiments } from 'src/statsig/types'
export const END_OF_ONBOARDING_SCREENS = [Screens.WalletHome, Screens.ChooseYourAdventure]

interface NavigatorFunctions {
navigate: typeof NavigationService.navigate | ((screen: Screens) => void)
Copy link
Contributor Author

@satish-ravi satish-ravi Jan 31, 2024

Choose a reason for hiding this comment

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

this dummy type (for calculating total steps) wasn't allowing setting screen props, updated the fn below to match navigate's type

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (36e6e66) 85.37% compared to head (c04805b) 85.37%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4834   +/-   ##
=======================================
  Coverage   85.37%   85.37%           
=======================================
  Files         728      728           
  Lines       29520    29522    +2     
  Branches     5173     5172    -1     
=======================================
+ Hits        25202    25204    +2     
  Misses       4072     4072           
  Partials      246      246           
Files Coverage Δ
...onsumerIncentives/ConsumerIncentivesHomeScreen.tsx 94.19% <100.00%> (ø)
src/home/NotificationBox.tsx 91.57% <100.00%> (ø)
src/onboarding/steps.ts 96.66% <100.00%> (+0.02%) ⬆️
src/send/SelectRecipientButtons.tsx 92.85% <100.00%> (ø)
src/RevokePhoneNumber.tsx 98.00% <0.00%> (ø)
src/account/Settings.tsx 85.34% <0.00%> (ø)
src/send/Send.tsx 93.18% <0.00%> (ø)
src/verify/VerificationStartScreen.tsx 91.80% <90.90%> (+0.06%) ⬆️

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 36e6e66...c04805b. Read the comment docs.

Copy link
Collaborator

@MuckT MuckT left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for cleaning up hideOnboardingStep logic! isOnboarding is a lot clearer.

@satish-ravi satish-ravi enabled auto-merge (squash) February 2, 2024 20:02
@satish-ravi satish-ravi merged commit 5548c88 into main Feb 2, 2024
16 checks passed
@satish-ravi satish-ravi deleted the satish/act-863-2 branch February 2, 2024 21:11
satish-ravi added a commit that referenced this pull request Feb 8, 2024
)

### Description

Fixes a bug introduced in #4834 , where VerificationStartScreen
defaulted to being not in onboarding. This causes an issue if a user
bails on the screen during onboarding. On restart, the screen is loaded
in the already onboarded mode where the back button does nothing and
there's no option to skip (see before video below). This PR reverts the
default to be in onboarding as it doesn't seem trivial to pass props on
the initialRoute.

### Test plan

Unit tests and manual

Before:


https://github.com/valora-inc/wallet/assets/5062591/496876ec-4adf-48b8-8a12-72d9030f3e37


After:


https://github.com/valora-inc/wallet/assets/5062591/8d345432-4fcb-439f-b63a-03806d5959e0


### Related issues

N/A

### Backwards compatibility

Yes
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
…lora-inc#4876)

### Description

Fixes a bug introduced in valora-inc#4834 , where VerificationStartScreen
defaulted to being not in onboarding. This causes an issue if a user
bails on the screen during onboarding. On restart, the screen is loaded
in the already onboarded mode where the back button does nothing and
there's no option to skip (see before video below). This PR reverts the
default to be in onboarding as it doesn't seem trivial to pass props on
the initialRoute.

### Test plan

Unit tests and manual

Before:


https://github.com/valora-inc/wallet/assets/5062591/496876ec-4adf-48b8-8a12-72d9030f3e37


After:


https://github.com/valora-inc/wallet/assets/5062591/8d345432-4fcb-439f-b63a-03806d5959e0


### Related issues

N/A

### Backwards compatibility

Yes
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