Adding recovery phrase video to onboarding process by ryanml · Pull Request #10717 · MetaMask/metamask-extension

ryanml · 2021-03-25T09:49:18Z

Related: #10244

Manual testing steps:

Clean install MetaMask
Select the "Create Account Route"
Confirm after creating a password you are directed to the seed phrase video page
Confirm that clicking next further continues on to the reveal seed phrase page
Confirm that this component is not shown in the "Import Account flow" on fresh install"

kumavis · 2021-03-25T10:05:04Z

😎 😎 😎

kumavis · 2021-03-25T10:05:40Z

note: video is delivered as part of extension, adds ~3mb (seems reasonable)

kumavis · 2021-03-25T10:06:28Z

how to intlize video? is it possible to include and specify alternate audio tracks?

ryanml · 2021-03-25T10:09:27Z

how to intlize video? is it possible to include and specify alternate audio tracks?

@kumavis see https://consensys.slack.com/archives/GETEW1V4M/p1616604943116400 for relevant discussion, I think we'll follow up on this

metamaskbot · 2021-03-25T10:58:30Z

Builds ready [88a1873]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (604 ± 26 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	43	81	58	11	5
		domContentLoaded	477	748	602	53	26
		load	478	750	604	53	26
		domInteractive	477	747	602	53	26

ryanml · 2021-03-25T11:09:41Z

ui/app/pages/first-time-flow/seed-phrase/seed-phrase.component.js

    return (
      <DragDropContextProvider backend={HTML5Backend}>
-        <div className="first-time-flow__wrapper">
+        <div className={`first-time-flow__wrapper ${introClass}`}>


When we modify the rest of the onboarding flow to follow the new designs, we can just bake rules directly in to first-time-flow__wrapper. Presently, it doesn't play so nicely with the new video screen

metamaskbot · 2021-03-25T20:43:15Z

Builds ready [f0c6beb]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (690 ± 36 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	44	80	60	10	5
		domContentLoaded	579	836	689	75	36
		load	579	837	690	74	36
		domInteractive	578	836	687	75	36

rachelcope

Looks good to me!

brad-decker

Looks awesome, a few change reqs but overall very excited about this landing.

brad-decker · 2021-03-31T15:25:58Z

ui/app/pages/first-time-flow/seed-phrase/seed-phrase-intro/seed-phrase-intro.component.js

+// Routes
+import { INITIALIZE_SEED_PHRASE_ROUTE } from '../../../../helpers/constants/routes';
+
+export default class SeedPhraseIntro extends PureComponent {


[REQ] We don't want to extend the pattern of using class components and splitting into containers/components. Could you refactor this into a functional component?

Force of habit ;) Can do!

brad-decker · 2021-03-31T15:28:57Z

ui/app/pages/first-time-flow/seed-phrase/seed-phrase-intro/seed-phrase-intro.component.js

+
+    return (
+      <div className="seed-phrase-intro__sidebar">
+        <div className="seed-phrase-intro__sidebar-block">


Try to use <Box> for some of these divs. Box is a layout component with various props for controlling size/spacing/layout.

Using box where it seems to make sense now

brad-decker · 2021-03-31T15:29:27Z

ui/app/pages/first-time-flow/seed-phrase/seed-phrase-intro/seed-phrase-intro.component.js

+          </div>
+        </div>
+        <div className="seed-phrase-intro__sidebar-block">
+          <div className="seed-phrase-intro__sidebar-title">


Try using <Typography> and semantic html tags like heading tags/p tags/span tags where appropriate.

brad-decker · 2021-03-31T15:30:23Z

ui/app/pages/first-time-flow/seed-phrase/seed-phrase-intro/index.scss

+  &__sidebar {
+    padding: 15px;
+    border-radius: 8px;
+    border: 1px solid #d6d9dc;


Don't use hex codes, match them to the color from the design system colors file. In this case $ui-2. https://github.com/MetaMask/metamask-extension/blob/12c614cc14fa9a146170e1989726deb758da310b/ui/app/css/design-system/colors.scss#L78-L110

brad-decker · 2021-03-31T15:34:27Z

ui/app/pages/first-time-flow/seed-phrase/seed-phrase-intro/index.scss

+    }
+  }
+
+  &__title {


This class, for example, can be expressed with Typography

<Typography color={COLORS.BLACK} variant={TYPOGRAPHY.H1} boxProps={{ marginTop: 0, marginBottom: 4 }} >

Done, and this rule removed as a result

brad-decker

Oops, was meant to hit req changes ;P

metamaskbot · 2021-04-06T01:38:48Z

Builds ready [963d4ea]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (521 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	43	65	50	6	3
		domContentLoaded	315	597	519	87	42
		load	316	598	521	87	42
		domInteractive	315	596	518	87	42

ryanml · 2021-04-11T00:50:21Z

Changes pushed cc: @brad-decker

metamaskbot · 2021-04-11T01:20:41Z

Builds ready [b2a29b5]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (631 ± 72 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	48	85	62	9	4
		domContentLoaded	378	805	630	150	72
		load	379	806	631	149	72
		domInteractive	378	804	630	150	72

brad-decker

LGTM, just a question

brad-decker · 2021-04-12T15:18:20Z

ui/app/pages/first-time-flow/seed-phrase/seed-phrase-intro/seed-phrase-intro.component.js

+              </ul>
+            </Box>
+            <Box marginBottom={4}>
+              <span className="seed-phrase-intro__sidebar-title">


Question: Why could the sidebar-title class items not be converted into Typography components?

seed-phrase-intro__sidebar-title css removed and elements converted to use the Typography component 👍

Adding english subtitles

metamaskbot · 2021-04-12T18:07:07Z

Builds ready [512393b]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (571 ± 48 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	47	84	57	11	5
		domContentLoaded	348	659	569	101	48
		load	350	660	571	100	48
		domInteractive	348	658	569	101	48

metamaskbot · 2021-04-13T19:15:31Z

Builds ready [a61a66a]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (585 ± 45 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	46	70	57	7	4
		domContentLoaded	376	685	584	95	45
		load	378	686	585	94	45
		domInteractive	376	685	584	95	45

brad-decker

LGTM!

ryanml requested a review from a team as a code owner March 25, 2021 09:49

ryanml self-assigned this Mar 25, 2021

ryanml requested a review from shanejonas March 25, 2021 09:49

ryanml force-pushed the seed-phrase-video branch 2 times, most recently from c2d4231 to 88a1873 Compare March 25, 2021 10:42

ryanml commented Mar 25, 2021

View reviewed changes

danjm requested a review from rachelcope March 25, 2021 17:23

ryanml force-pushed the seed-phrase-video branch from 88a1873 to f0c6beb Compare March 25, 2021 20:26

rachelcope previously approved these changes Mar 25, 2021

View reviewed changes

brad-decker approved these changes Mar 31, 2021

View reviewed changes

brad-decker suggested changes Mar 31, 2021

View reviewed changes

ryanml dismissed stale reviews from ghost and rachelcope via 963d4ea April 6, 2021 01:22

ryanml force-pushed the seed-phrase-video branch from f0c6beb to 963d4ea Compare April 6, 2021 01:22

ryanml force-pushed the seed-phrase-video branch from 963d4ea to e0438df Compare April 11, 2021 00:48

ryanml requested review from brad-decker and rachelcope April 11, 2021 00:49

ryanml force-pushed the seed-phrase-video branch 2 times, most recently from db60d3d to b2a29b5 Compare April 11, 2021 01:04

brad-decker previously approved these changes Apr 12, 2021

View reviewed changes

Adding recovery phrase video to onboarding process

512393b

Adding english subtitles

ryanml dismissed brad-decker’s stale review via 512393b April 12, 2021 17:47

ryanml force-pushed the seed-phrase-video branch from b2a29b5 to 512393b Compare April 12, 2021 17:47

ryanml requested a review from brad-decker April 12, 2021 17:48

Support textAlign in Box, converting sidebar to Box

a61a66a

brad-decker approved these changes Apr 13, 2021

View reviewed changes

ryanml merged commit 9e918b6 into develop Apr 13, 2021

ryanml deleted the seed-phrase-video branch April 13, 2021 22:29

github-actions bot locked and limited conversation to collaborators Apr 13, 2021

Conversation

ryanml commented Mar 25, 2021

Uh oh!

kumavis commented Mar 25, 2021

Uh oh!

kumavis commented Mar 25, 2021

Uh oh!

kumavis commented Mar 25, 2021

Uh oh!

ryanml commented Mar 25, 2021

Uh oh!

metamaskbot commented Mar 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 25, 2021

Uh oh!

rachelcope left a comment

Choose a reason for hiding this comment

Uh oh!

brad-decker 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!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Apr 6, 2021

Uh oh!

ryanml commented Apr 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metamaskbot commented Apr 11, 2021

Uh oh!

brad-decker 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!

metamaskbot commented Apr 12, 2021

Uh oh!

metamaskbot commented Apr 13, 2021

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ryanml commented Apr 11, 2021 •

edited

Loading