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

Reminder to backup seed phrase #1892

Merged
merged 67 commits into from
Nov 3, 2020
Merged

Conversation

rickycodes
Copy link
Contributor

@rickycodes rickycodes commented Oct 15, 2020

Description

Add reminder to backup seed phrase. This PR encompasses a whole slew of changes, I am going to list them all out here:

  1. If a user has not gone through the protect wallet process they'll see a notification on the drawer now:
    image
  2. similarity there's a notification under Security & Privacy in settings:
    image
  3. once in the Security & Privacy section they'll be presented with an option to "Back up now". this will be visible even if the user dismisses the yellow snack bar prompt we display:
    image
    4: there's also an option to reset password:
    image
  4. The order of the items in the Security & Privacy area is also changed, and now includes Sub headings for Security and for Privacy.
  5. once the user has backed up their wallet it should look like this:
    image
  6. if the user left themselves a hint, they'll have an option to view and edit it:
    image

There's also a list of supplementary changes that weren't necessarily related to the changes in figma...

adjusted spacing for the skip modal

before

image

after

image

adjusted line height on "i understand..." checkbox when creating a password

before:

image

after:

image

we've removed one of the steps ("Maintain a public aggregate dashboard to educate the community") from the the opt-in screen:

image

there's also some other things I am forgetting, they'll come back to me and I'll update them here...

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #???

@rickycodes rickycodes force-pushed the feature/reminder-backup-seed-phrase branch 3 times, most recently from 5972248 to 89c1a70 Compare October 19, 2020 23:45
@rickycodes rickycodes changed the title Add protect reminder section to settings Reminder to backup seed phrase Oct 20, 2020
@rickycodes rickycodes force-pushed the feature/reminder-backup-seed-phrase branch 3 times, most recently from cf2cbdc to d83288f Compare October 26, 2020 04:47
@@ -513,7 +516,7 @@ class ChoosePassword extends PureComponent {
} = this.state;
const passwordsMatch = password !== '' && password === confirmPassword;
const canSubmit = passwordsMatch && isSelected;
const previousScreen = this.props.navigation.getParam(AppConstants.PREVIOUS_SCREEN);
const previousScreen = this.props.navigation.getParam(PREVIOUS_SCREEN);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has been wrong for a while...

PREVIOUS_SCREEN is no longer in AppConstants 🤦‍♀️

@@ -24,7 +24,7 @@ import Device from '../../../util/Device';
import Icon from 'react-native-vector-icons/Octicons';
import Confetti from '../../UI/Confetti';
import { getOnboardingNavbarOptions } from '../../UI/Navbar';
import { ONBOARDING_WIZARD, METRICS_OPT_IN } from '../../../constants/storage';
import { ONBOARDING_WIZARD, METRICS_OPT_IN, SEED_PHRASE_HINTS } from '../../../constants/storage';
Copy link
Contributor Author

@rickycodes rickycodes Oct 26, 2020

Choose a reason for hiding this comment

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

made this into a constant... it's a shame we broke our convention of @MetaMask:* for this key... we should try not to do this in the future.

@rickycodes rickycodes force-pushed the feature/reminder-backup-seed-phrase branch 3 times, most recently from 36169ff to 158e934 Compare October 27, 2020 04:49
@rickycodes rickycodes marked this pull request as ready for review October 27, 2020 16:40
@rickycodes rickycodes requested a review from a team as a code owner October 27, 2020 16:40
@rickycodes rickycodes added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 27, 2020
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Issue 1:

As soon as I install the app, and create a wallet, and skip backup, then go to settings > security & privacy, I get this warning

image

Issue 2:

When I'm in the above state, (I install the app, and create a wallet, and skip backup, then go to settings > security & privacy) and tap back up now CTA android immediately asks me for my password, however, iOS doesn't ask me for my password

seen here = http://recordit.co/IRyQlm8pcT

Something weird though, when I have a real device with biometrics enabled (FaceID) I do see it ask me to scan my face; seen here = http://recordit.co/MX1VgM3ykH

Also, when you do end up actually backing up your seedphrase, and then tap on back up again, it does indeed ask for password on all scenarios (biometrics or no biometrics) seen here = http://recordit.co/XqoR5unIk1

Issue 2A:

In addition, the left title is overlapping with the METAMASK title on iOS

image

Issue 3:

When attempting to change password, the first view asks to enter current password, however, on devices I have biometrics enabled I don’t see the option to use biometrics to pass this step, is that intended?

seen here = http://recordit.co/PPPrnECo8k

Issue 4:

I don’t see the alert that my password was updated on Android

Seen here = http://recordit.co/DayEQ6m35F

Issue 5:

Once backing up again and going through the flow, the hint shows up as empty, but when going back to settings after completing flow, the hint is still there

Seen here = http://recordit.co/U24uQ01XWG

Issue 6:

So if I have a fresh install of the app and import my wallet, I don't see the congratulations confetti screen, however, if I already have a wallet installed, log out, and then go to import via this flow, it does work.

Seen here = http://recordit.co/uMDqWOT1Xw

@ibrahimtaveras00 ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 28, 2020
@rickycodes rickycodes force-pushed the feature/reminder-backup-seed-phrase branch 2 times, most recently from 5d237bb to 87195aa Compare October 29, 2020 16:45
@ibrahimtaveras00
Copy link
Contributor

ibrahimtaveras00 commented Oct 29, 2020

Issue 7:

Change password should be secondary button outline

Screen Shot 2020-10-29 at 4 48 57 PM

Issue 8:

If we could use smaller font and left align the copy here (to match font sizes of next screen: cc @cjeria)

Screen Shot 2020-10-29 at 4 45 55 PM

Also we're repeating the change password text on this screen

Screen Shot 2020-10-29 at 4 47 03 PM

Also, the button fonts do not match the mock up

@swornov
Copy link

swornov commented Oct 30, 2020 via email

@rickycodes rickycodes force-pushed the feature/reminder-backup-seed-phrase branch 2 times, most recently from 6e56502 to dc948f5 Compare October 31, 2020 18:39
@rickycodes
Copy link
Contributor Author

I don't think we should be addressing any design related issues in this PR. Like centring the text for the password confirmation screen... I am merely reusing components as they appear in the application today... if those components need to be tweaked or redesigned they should be addressed in a separate PR. there's already a bunch of changes in here that are outside the scope of the original task.

@cjeria
Copy link

cjeria commented Nov 2, 2020

@rickycodes totally understand, however, the original task/design suggests specific UI patterns that do fit into the scope. Also, we're not suggesting we change the original components since they work in other parts of the app so those should likely not be changed, rather we might need to add variations of those components so that they work in different contexts.

those components need to be tweaked or redesigned they should be addressed in a separate PR.

Sounds good. I'd suggest we make these tweaks next so we don't forget about them. Some of those screens look kinda funky. cc @omnat WDYT?

@rickycodes rickycodes force-pushed the feature/reminder-backup-seed-phrase branch from 73f05a7 to 9208edf Compare November 2, 2020 18:44
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

👍

@rickycodes rickycodes force-pushed the feature/reminder-backup-seed-phrase branch from b14c3c8 to 3321e99 Compare November 3, 2020 16:35
@andrepimenta andrepimenta removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Nov 3, 2020
@rickycodes rickycodes merged commit 8068314 into develop Nov 3, 2020
@rickycodes rickycodes deleted the feature/reminder-backup-seed-phrase branch November 3, 2020 19:42
rickycodes added a commit that referenced this pull request Jan 31, 2022
* Add protect reminder section to settings

* Add SettingsWarning component

* Use children propType for SettingsWarning

* Update propTypes

* Add warning to Settings in drawer

* Unify into one component

* cleanup

* Add seedphraseBackedUp check

* Add drillIntoSecuritySettings method

* add to locales

* Update functionality based on call from earlier today

* Rename SettingsWarning to SettingsNotification

* Place notification on second settings pane

* Remove "Maintain a public aggregate dashboard to educate the community" from opt-in list

* Fix spacing

* Complete "protect your wallet"

* Add password reset text to settings

* Add ResetPassword View

* Move ResetPassword into seperate navigator

* Get password reset functionality working

* Update tests

* code cleanup

* Update Settings test

* fix

* Update snap

* Clean up componentDidMount()

* Fix PREVIOUS_SCREEN bug

* Move ResetPassword into MainNavigator

* Add manual backup steps to MainNavigator

* Show toast after password is updated

* Change wording of success toast

* Re-order SecuritySettings and add headings

* Add warning icon to title

* Display ActionModal again when snack bar is dismissed

* Create SkipAccountSecurityModal component

* Add defaultProps

* Add recovery hint modal

* Update snapshots

* Use optional chaining

* Add ManualBackup to Main

* Show congratulations screen on seed phrase import

* Update snapshots

* Adjust lineHeight and checkbox position

* Add HintModal

* Update snapshots

* Update translations

* Remove SEED_PHRASE_HINTS where appropriate

* Add cancelledBiometricsPermission method

* Update translations

* Fix isRequired warning for HintModal

* display hint it if set on congratulations screen

* Update title to be just "Back" when in Security & Privacy

* Update styles

* Display congrats screen for !metricsOptIn case

* Update snapshot

* Align check box to top

* Make backUpSeedphraseAlertNotVisible required

* Add comments for warning prop

* Add tests for new components

* Vertically align items in SettingsNotification

* Add test for SkipAccountSecurityModal

* Fix css

* Fix font issues

* Address PR feedback

* Update snapshots

* Use getOnboardingNavbarOptions for ResetPassword

* Update snapshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release QA Passed A successful QA run through has been done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants