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(CAB): Add landing screen for phone verification (recovery) #4821

Merged
merged 16 commits into from
Feb 5, 2024

Conversation

finnian0826
Copy link
Contributor

@finnian0826 finnian0826 commented Jan 29, 2024

Description

Add in link phone number landing screen. This will come after successfully completing the recovery flow. Not hooked up yet since that work isn't done.

Test plan

Unit tests added.

landing_screen.mp4

Related issues

Backwards compatibility

Yes, just adding new screen.

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

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

Comparison is base (871e186) 85.37% compared to head (6428132) 85.40%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4821      +/-   ##
==========================================
+ Coverage   85.37%   85.40%   +0.02%     
==========================================
  Files         728      729       +1     
  Lines       29522    29562      +40     
  Branches     5172     5175       +3     
==========================================
+ Hits        25203    25246      +43     
+ Misses       4073     4070       -3     
  Partials      246      246              
Files Coverage Δ
src/analytics/Events.tsx 100.00% <100.00%> (ø)
src/analytics/Properties.tsx 100.00% <ø> (ø)
src/components/Button.tsx 100.00% <100.00%> (ø)
src/navigator/Screens.tsx 100.00% <100.00%> (ø)
src/keylessBackup/LinkPhoneNumber.tsx 96.42% <96.42%> (ø)

... and 2 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 871e186...6428132. Read the comment docs.

@finnian0826 finnian0826 marked this pull request as ready for review January 30, 2024 21:32
Comment on lines 46 to 49
style={{
padding: Spacing.Thick24,
alignItems: 'center',
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: assign this style a variable name and put it with the others

size={BtnSizes.FULL}
testID="LinkPhoneNumberButton"
/>
<TextButton
Copy link
Contributor

@jophish jophish Jan 30, 2024

Choose a reason for hiding this comment

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

Looking at the designs here, it looks like this secondary button is styled differently. (Also the primary button is a different shade of green.) I see that this PR follows the designs shown in the Figma preview in the Linear ticket - should the designs in Figma be the source of truth here? (or am I looking at the wrong designs in Figma?)


useLayoutEffect(() => {
navigation.setOptions({
headerLeft: () => <BackButton />,
Copy link
Contributor

Choose a reason for hiding this comment

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

From the image you posted, it looks like the header background is a different color than the page background, which doesn't match the designs.

<Button
text={t('linkPhoneNumber.startButtonLabel')}
onPress={() => navigate(Screens.VerificationStartScreen, { hideOnboardingStep: true })}
style={{ marginBottom: Spacing.Thick24, width: '100%' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: bring styles out into style object

<TextButton
testID="LinkPhoneNumberLater"
style={{ color: colors.primary }}
onPress={() => navigateHome()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an analytics event for both buttons?

@finnian0826 finnian0826 enabled auto-merge (squash) February 5, 2024 17:37
@finnian0826 finnian0826 disabled auto-merge February 5, 2024 18:18
@finnian0826 finnian0826 enabled auto-merge (squash) February 5, 2024 18:18
@finnian0826 finnian0826 merged commit fe2e559 into main Feb 5, 2024
15 checks passed
@finnian0826 finnian0826 deleted the finnian0826/link-phone-number branch February 5, 2024 19:13
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
…ra-inc#4821)

### Description

Add in link phone number landing screen. This will come after
successfully completing the recovery flow. Not hooked up yet since [that
work](https://linear.app/valora/issue/ACT-781/recovery-success-screen)
isn't done.

### Test plan

Unit tests added.


https://github.com/valora-inc/wallet/assets/140328381/1f1e14ca-bbb6-4453-871e-d85769b4f141


### Related issues

- Fixes #[ACT-768]

### Backwards compatibility

Yes, just adding new screen.
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.

4 participants