First time flow updates by danjm · Pull Request #6192 · MetaMask/metamask-extension

danjm · 2019-02-20T20:24:04Z

Final design decisions pending. Tests will be updated once design decisions finalized.

…e flow.

metamaskbot · 2019-02-26T16:36:00Z

Builds ready [68cdc38]: mascara, chrome, firefox, edge, opera

whymarrh

I left a few comments inline. Also, if we're removing the notices should we also remove the components and files (the notices/ directory?) associated with them?

whymarrh · 2019-02-26T16:58:32Z

app/_locales/en/messages.json

+    "message": "Be careful with your seed phrase — there have been reports of websites that attempt to imitate MetaMask. MetaMask will never spontaneously ask for your seed phrase!"
+  },
+  "protectYourKeysMessage2": {
+    "message": "Keep your phrase safe. If you see something phishy, or you’re uncertain about a website, email support@metamask.io."


Should "phishy" here be "fishy"? Fishy seems like actual word^[1][2][3][4] for this context?

Also, do we want to have the period at the end of the sentence following the email address? I don't have a strong opinion either way, but it could be confusing to some users who will inevitably copy and paste the address with a period.

Done in e7b8ea021

whymarrh · 2019-02-26T16:59:59Z

test/e2e/beta/run-all.sh

 export PATH="$PATH:./node_modules/.bin"

-shell-parallel -s 'npm run ganache:start -- -b 2' -x 'sleep 5 && static-server test/e2e/beta/contract-test --port 8080' -x 'sleep 5 && mocha test/e2e/beta/metamask-beta-ui.spec'
+# shell-parallel -s 'npm run ganache:start -- -b 2' -x 'sleep 5 && static-server test/e2e/beta/contract-test --port 8080' -x 'sleep 5 && mocha test/e2e/beta/metamask-beta-ui.spec'


Why are we disabling these tests?

Fixed in ac872717b

whymarrh · 2019-02-26T17:02:31Z

...first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.component.js

  }

+  toggleTermsCheck = () => {
+    this.setState({


We should prefer the updater function argument here. That is,

this.setState((prevState) => ({ termsChecked: !prevState.termsChecked, })

Done in ac872717b

whymarrh · 2019-02-26T17:02:56Z

...first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.component.js

+            {termsChecked ? <i className="fa fa-check fa-2x" /> : null}
+          </div>
+          <span className="first-time-flow__checkbox-label">
+            I agree to the Terms Of Service


Should we extract this into a message string?

Done in e7b8ea021

whymarrh · 2019-02-26T17:03:32Z

ui/app/components/pages/first-time-flow/create-password/new-account/new-account.component.js

  }

+  toggleTermsCheck = () => {
+    this.setState({


We should prefer the updater function argument here. That is,

this.setState((prevState) => ({ termsChecked: !prevState.termsChecked, })

Fixed in 51fb72b40

whymarrh · 2019-02-26T17:04:27Z

ui/app/components/pages/first-time-flow/create-password/new-account/new-account.component.js

+              {termsChecked ? <i className="fa fa-check fa-2x" /> : null}
+            </div>
+            <span className="first-time-flow__checkbox-label">
+              I agree to the Terms Of Service


Should we extract this into a message string?

Done in e7b8ea021

whymarrh · 2019-02-26T17:05:19Z

ui/app/components/pages/first-time-flow/create-password/unique-image/unique-image.component.js

+          type="confirm"
          className="first-time-flow__button"
-          onClick={() => history.push(INITIALIZE_NOTICE_ROUTE)}
+          onClick={async () => {


Why is this handler async?

Fixed in ac872717b

whymarrh · 2019-02-26T17:06:46Z

ui/app/components/pages/first-time-flow/end-of-flow/end-of-flow.component.js

+            history.push(DEFAULT_ROUTE)
+          }}
+        >
+          { 'All Done' }


Can we inline all of the strings in this component? Or maybe we should even extract them into message strings?

Fixed in e7b8ea021

whymarrh · 2019-02-26T17:08:18Z

ui/app/components/pages/first-time-flow/select-action/select-action.component.js

+import React, { PureComponent } from 'react'
+import PropTypes from 'prop-types'
+import Button from '../../../button'
+import { INITIALIZE_CREATE_PASSWORD_ROUTE, INITIALIZE_NOTICE_ROUTE, INITIALIZE_IMPORT_WITH_SEED_PHRASE_ROUTE } from '../../../../routes'


Nitpick: can we split this import onto multiple lines?

Fixed in ac872717b

whymarrh · 2019-02-26T17:09:07Z

ui/app/components/pages/first-time-flow/select-action/select-action.component.js

+                  { 'Import Wallet' }
+                </div>
+                <div className="select-action__button-text-small">
+                  { 'Import your existing wallet using a 12 word seed phrase' }


Should we extract the messages in this component into message strings?

Done in e7b8ea021

danjm · 2019-02-27T05:01:48Z

@whymarrh noticed related code removed in 275f7c247

metamaskbot · 2019-02-27T05:07:12Z

Builds ready [275f7c2]: mascara, chrome, firefox, edge, opera

whymarrh · 2019-02-27T13:38:44Z

app/_locales/en/messages.json

Nitpick: trailing space

whymarrh · 2019-02-27T13:39:03Z

app/_locales/en/messages.json

Nit: can we factor out the bullet?

whymarrh · 2019-02-27T13:39:10Z

app/_locales/en/messages.json

Nit: can we factor out the bullet?

whymarrh · 2019-02-27T13:39:20Z

app/_locales/en/messages.json

Nit: can we factor out the bullet?

Nit: also s/Metamask/MetaMask/

whymarrh · 2019-02-27T13:40:22Z

app/_locales/en/messages.json

Nit: s/find them/find it/ I think? Or maybe s/back it up/back them up/ if that works?

whymarrh · 2019-02-27T13:41:43Z

app/_locales/en/messages.json

Nit: should we drop "spontaneously"? Below we have yourUniqueAccountImageDescription3 plainly saying never.

…option.

metamaskbot · 2019-02-27T14:17:35Z

Builds ready [5771a6f]: mascara, chrome, firefox, edge, opera

danjm requested a review from whymarrh as a code owner February 20, 2019 20:24

danjm added the DO-NOT-MERGE Pull requests that should not be merged label Feb 20, 2019

danjm mentioned this pull request Feb 21, 2019

Metametrics #6171

Merged

6 tasks

danjm added 11 commits February 26, 2019 12:26

Action select step of onboarding flow added.

8f9b447

Update navigation on create and import password screens.

2f875fc

Adds terms of service checkbox to create and import account screens.

76768cc

Add security warning to jazzicon intro step

46d5f4f

Update and streamline unique image to confirm seed steps of first tim…

8933b0e

…e flow.

UI touch ups to welcome screen.

b39f720

UI touch up on select action page

2b0a9f4

Fix first time import flow.

dd2a8be

Add end of flow screen to first time flow

94de29d

Replace unique image screen with updated fishing warning screen.

31b2b17

Update e2e tests for onboarding flow changes.

68cdc38

danjm force-pushed the onboarding-flow-updates branch from a15c406 to 68cdc38 Compare February 26, 2019 16:28

danjm removed the DO-NOT-MERGE Pull requests that should not be merged label Feb 26, 2019

whymarrh suggested changes Feb 26, 2019

View reviewed changes

whymarrh suggested changes Feb 27, 2019

View reviewed changes

danjm added 5 commits February 27, 2019 10:34

Add required translations to onboarding flow.

a91504a

Update design of select action screen to emphasize create new wallet …

440b91f

…option.

Clean up onboarding flow code.

4754ec3

Remove notice related code from first-time-flow directory.

dda4fde

Use updater function argument in new-account.component

5771a6f

danjm force-pushed the onboarding-flow-updates branch from 51fb72b to 5771a6f Compare February 27, 2019 14:04

whymarrh approved these changes Feb 27, 2019

View reviewed changes

whymarrh merged commit cb2698d into develop Feb 27, 2019

whymarrh deleted the onboarding-flow-updates branch February 27, 2019 14:46

bdresser mentioned this pull request Feb 28, 2019

UI fixes to new Onboarding flow #6222

Closed

tmashuang mentioned this pull request Jun 28, 2019

Restore your account: Back button doesn't work #6771

Closed

Conversation

danjm commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metamaskbot commented Feb 26, 2019

Uh oh!

whymarrh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danjm commented Feb 27, 2019

Uh oh!

metamaskbot commented Feb 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danjm commented Feb 20, 2019 •

edited

Loading